Add reprojection/mosaic pipeline, mosaic viewer UI, and CLI#106
Merged
rfrenchseti merged 26 commits intomainfrom Apr 26, 2026
Merged
Add reprojection/mosaic pipeline, mosaic viewer UI, and CLI#106rfrenchseti merged 26 commits intomainfrom
rfrenchseti merged 26 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new nav.reproj package (body/ring reprojection, mosaics, photometric/cartographic/orbit models, serialization), CLI drivers and cloud‑tasks workers for mosaics, PyQt6 mosaic viewers, masked‑NCC updates, logger refactors (IMAGE_LOGGER/MAIN_LOGGER), dataset/config validation, documentation, and tests. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #106 +/- ##
==========================================
- Coverage 42.36% 36.14% -6.23%
==========================================
Files 70 90 +20
Lines 6446 12431 +5985
Branches 916 1696 +780
==========================================
+ Hits 2731 4493 +1762
- Misses 3509 7562 +4053
- Partials 206 376 +170 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 47
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/nav/support/image.py (1)
256-271:⚠️ Potential issue | 🟠 MajorAdd input validation and refresh the stale docstring for
array_zoom.Two concerns on the rewritten body:
- Missing boundary validation (public API). Sibling helpers in this module (
shift_array,pad_array,unpad_array) all checkarray.ndim == len(<shape-arg>)and raiseValueError.array_zoomnow silently tolerates mismatches:
len(factor) < a.ndim→ trailing axes aren't zoomed and the caller gets an array with an unexpected shape.len(factor) > a.ndim→np.repeatraises a crypticAxisErrormid-loop.f == 0,f < 0, or non-integerf(e.g.0.5) all slip throughif f > 1as no-ops (or get truncated bynp.repeat) instead of being rejected.- The docstring at lines 257-265 still says "using array indexing", which no longer describes the
np.repeat-based implementation, and it doesn't mention the integer-factor requirement or theRaises:behavior.As per coding guidelines: "Validate inputs at the public API boundary of the library. Raise clear
ValueErrororTypeErrorexceptions for invalid arguments." and "ALWAYS update docstrings when the associated code changes."🛠️ Proposed fix
def array_zoom(a: NDArrayType[NPType], factor: list[int] | tuple[int, ...]) -> NDArrayType[NPType]: - """Zooms an array by the specified factor using array indexing. + """Zoom an array by repeating elements along each axis. Parameters: a: The input array to zoom. - factor: The zoom factor for each dimension of the array. + factor: The integer zoom factor for each dimension of the array. Must have + the same length as ``a.ndim``. Each factor must be >= 1; a factor of 1 + leaves the corresponding axis unchanged. Returns: The zoomed array with dimensions multiplied by the corresponding factors. + + Raises: + ValueError: If ``len(factor) != a.ndim`` or any factor is less than 1. + TypeError: If any factor is not an integer. """ - result: np.ndarray = np.asarray(a) - for ax, f in enumerate(factor): - if f > 1: - result = np.repeat(result, f, axis=ax) - return cast(NDArrayType[NPType], result) + result: np.ndarray = np.asarray(a) + if len(factor) != result.ndim: + raise ValueError( + f'Array ({result.ndim}) and factor ({len(factor)}) must have same ' + f'number of dimensions' + ) + for ax, f in enumerate(factor): + if not isinstance(f, (int, np.integer)): + raise TypeError(f'factor[{ax}] must be an integer, got {type(f).__name__}') + if f < 1: + raise ValueError(f'factor[{ax}] must be >= 1, got {f}') + if f > 1: + result = np.repeat(result, f, axis=ax) + return cast(NDArrayType[NPType], result)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nav/support/image.py` around lines 256 - 271, array_zoom currently omits input validation and has an outdated docstring; update array_zoom's docstring to describe the np.repeat-based implementation, require integer zoom factors >=1, and document the Raises behavior, then add explicit checks at the function start that a.ndim == len(factor) (mirror shift_array/pad_array/unpad_array), validate that each element of factor is an int and >= 1 (raise TypeError for non-int, ValueError for <1), and raise a clear ValueError if lengths mismatch so callers get deterministic, helpful errors rather than AxisError or silent no-ops.src/nav/dataset/dataset_pds3.py (1)
592-735:⚠️ Potential issue | 🟡 MinorCase-sensitive filter can silently drop every match.
Index-derived
img_namevalues are upper-case (the Cassini ISS index enforces.IMG/N|Wprefixes in upper case), but the CSV path throughyield_image_files_from_argumentsdoes not normalize case forimage_filespec_csventries (line 371), and neither_get_img_name_from_label_filespecnor this loop upper-cases the resolved filter names. A user CSV with lowercase filespecs will produce a non-emptyimg_name_filter_listcontaining lowercase names, so theincheck at Line 734 silently excludes every row — no warning, just an empty result. This is also asymmetric with theimg_name_listmatch at Line 740, which compares via.lower().Suggest normalizing once during resolution so the membership check is deterministic:
🛡️ Proposed fix
if img_name_filter_list: new_img_name_filter_list: list[str] = [] for explicit_img_filespec in img_name_filter_list: new_img_name = self._get_img_name_from_label_filespec(explicit_img_filespec) if new_img_name is None: continue - new_img_name_filter_list.append(new_img_name) + new_img_name_filter_list.append(new_img_name.upper()) img_name_filter_list = new_img_name_filter_listAnd at Line 734, compare upper-to-upper to tolerate any index casing drift:
- if img_name_filter_list and img_name not in img_name_filter_list: + if img_name_filter_list and img_name.upper() not in img_name_filter_list: continueUsing a
setforimg_name_filter_listwould also turn the O(n) membership into O(1) for large CSVs, while you're here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nav/dataset/dataset_pds3.py` around lines 592 - 735, The img_name_filter_list from yield_image_files_from_arguments is not normalized to the same case as index-derived img_name, so membership checks in the loop (img_name_filter_list vs img_name in _get_img_name_from_label_filespec) can silently miss matches; change resolution in yield_image_files_from_arguments to normalize each CSV-resolved filespec via _get_img_name_from_label_filespec(...).upper() (or otherwise call .upper() on the returned name) and store them in a set (img_name_filter_list -> img_name_filter_set) to both normalize casing and make membership O(1), then in this loop compare img_name.upper() against that set (or ensure both sides are uppercased) when checking inclusion.src/nav/support/correlate.py (2)
582-591:⚠️ Potential issue | 🟠 MajorReturn the same keys in the no-candidate fallback.
This branch omits
peak_val, butnavigate_with_pyramid_kpeaks()readsres_lvl["peak_val"]unconditionally on Line 763. When every shift is invalidated, the fallback now turns into aKeyErrorinstead of a clean low-quality result.Proposed fix
if not candidates: # TODO When no candidates are found, the function returns cov: np.diag([1e6, 1e6]) # and quality: -np.inf. Downstream code might not check for -np.inf quality and could # treat this as a valid result. Consider returning None or raising an exception instead. return { 'offset': (0.0, 0.0), 'cov': np.diag([1e6, 1e6]), 'sigma_xy': (1e3, 1e3), 'quality': -np.inf, + 'peak_val': -np.inf, + 'rc': (0, 0), }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nav/support/correlate.py` around lines 582 - 591, The no-candidate fallback in correlate.py returns a dict missing the 'peak_val' key, causing navigate_with_pyramid_kpeaks() (which reads res_lvl["peak_val"] unconditionally) to raise KeyError; update the fallback returned by the candidates-empty branch to include a 'peak_val' entry (set to a sensible low/zero value consistent with existing semantics, e.g., 0.0 or -np.inf) alongside 'offset', 'cov', 'sigma_xy', and 'quality' so res_lvl always contains the same keys expected by navigate_with_pyramid_kpeaks() and downstream code.
404-421:⚠️ Potential issue | 🟠 MajorRefine against the NCC peak, not the raw numerator.
nms_topk()now selects candidates fromcorr, but this block still upsamples the unnormalized numerator spectrum. That reintroduces the same variance bias you just removed at integer-pixel resolution, so the final(dy, dx)can still walk off the correct NCC peak. Subpixel refinement needs to come from the NCC neighborhood itself, or from a locally re-normalized surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nav/support/correlate.py` around lines 404 - 421, The subpixel refinement is using the raw numerator spectrum `spec` (from image_pad/model_pad/mask_pad) which reintroduces bias; instead use the normalized NCC surface (`corr`) as the input to `upsampled_dft` so refinement is performed on the same normalized peak `nms_topk()` selected. Replace the `spec = fft2(image_pad) * np.conj(fft2(model_pad * mask_pad))` usage with a padded FFT of the normalized correlation (`corr`) region (e.g., build a padded `corr_pad` matching the current padding and call `spec = fft2(corr_pad)`), then pass that `spec` into `upsampled_dft(spec, upsample_factor, (region_v, region_u), (oy - dy_i * upsample_factor, ox - dx_i * upsample_factor))` so `Up`/`upy, upx` are computed from the normalized surface rather than the unnormalized numerator.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Around line 100-104: The sentence "None is safe for concurrent use on the same
`obs`" is ambiguous; change it to explicitly reference the three entry points
(e.g., "None of these is safe for concurrent use on the same `obs`" or "None of
them are safe for concurrent use on the same `obs`") so the antecedent includes
RingMosaic.reproject(), BodyMosaic.reproject(), and create_cartographic_model();
keep the surrounding mention of `_reduced_oops_precision` and `obs` intact for
context.
In `@CODEBASE_ANALYSIS.md`:
- Around line 1-188: The analysis doc's section numbering jumps (sections
2,3,5,6,7 missing) which can confuse readers; edit CODEBASE_ANALYSIS.md near the
top just before "## 1. Structure and layout" to add a single-line explanatory
note such as "Numbers follow the parent audit outline; sections without findings
are omitted" so readers know the omission is intentional; ensure the note is
concise and placed immediately above or below the document title/header so it is
visible when scanning the "## 1. Structure and layout" section.
- Line 1: Add a top-level H1 heading to the top of the document so the first
line is a top-level heading (fixes MD041); for example insert "# Codebase
Analysis" (or an appropriate document title) above the existing "## 1. Structure
and layout" heading so the file begins with a single top-level heading and
preserves the current subheading structure.
In `@docs/api_reference/api_reproj.rst`:
- Around line 4-32: The automodule blocks for nav.reproj (e.g., automodule::
nav.reproj.bodies, nav.reproj.rings, nav.reproj.cartographic_model,
nav.reproj.photometric_model, nav.reproj.ring_orbit_model) incorrectly include
the :no-index: option; remove the :no-index: lines from those automodule
directives so these modules are indexed by Sphinx and cross-references like
class `~nav.reproj.bodies.BodyMosaic` and method
`~nav.reproj.rings.RingMosaic.add` resolve correctly.
In `@docs/user_guide_backplanes.rst`:
- Around line 86-90: Update the phrasing that reads "per non-empty backplane
array" to remove ambiguity; change it to "per non-all-zero backplane array" (or
alternatively "per included backplane (see above)") so it aligns with the
earlier rule "Backplanes that are entirely zero are omitted" and keep the
surrounding references to ``BODY_ID_MAP`` and ``ImageHDU`` unchanged.
In `@docs/user_guide_reprojection.rst`:
- Around line 225-229: The example sets mean_motion using a low-precision
hand-coded π; replace the expression mean_motion=581.964 * 3.14159 / 180.0 with
a precise conversion such as mean_motion=math.radians(581.964) (or
np.deg2rad(581.964)) and add an import math at the top of the example block if
not already present; ensure the change is applied where the
RingOrbitModel/my_orbit example and the RingMosaic('SATURN', ...) usage appear
so copied user code uses the higher-precision conversion.
- Around line 718-721: The example filename for the body display command is
using the wrong segment order and omits the body, causing copy-paste failures;
update the example for the nav_mosaic_display_body invocation to follow the
project's naming scheme (<prefix>_<body_or_planet>_<image_stem>_reproj.<fmt>) as
implemented in src/reproj_cli/paths.py and earlier examples (lines 419/484) —
e.g., change the displayed filename to something like
mimas_2004_MIMAS_N1234567890_reproj.fits so the body segment appears before the
image stem and matches the reproj naming convention.
In `@pyproject.toml`:
- Around line 64-81: The pyproject 'docs' extra duplicates many runtime
dependencies (risking drift); remove the duplicated runtime packages from the
docs extra and instead make the docs extra depend on the package install so it
inherits runtime deps — update the 'docs' extras definition (the "docs" list) to
reference the project install rather than listing runtime libs, and ensure
ReadTheDocs/CI installs the package via the recommended pattern (installing the
project itself with extras, e.g. using .[docs]) so the 'dev' extra (and autodoc)
get the runtime dependencies transitively.
In `@src/backplanes/backplanes.py`:
- Around line 36-37: Remove the redundant local alias by deleting the line
"logger = IMAGE_LOGGER" and update any references in this module (e.g., uses in
functions or methods within backplanes.py) to use the module-level IMAGE_LOGGER
directly; confirm DEFAULT_CONFIG remains as-is and run tests/lints to ensure no
unresolved symbol "logger" remains.
- Line 8: The file imports IMAGE_LOGGER from the internal submodule
nav.config.logger which couples to module internals; change the import to use
the public re-export from nav.config (i.e., import IMAGE_LOGGER from nav.config)
so other call sites are consistent—update the import statement that references
IMAGE_LOGGER in backplanes.py to import from nav.config instead of
nav.config.logger.
In `@src/backplanes/writer.py`:
- Line 9: The import in src/backplanes/writer.py should use the public namespace
for logger singletons to match other modules; replace the current import of
IMAGE_LOGGER from nav.config.logger with an import from nav.config (i.e., import
IMAGE_LOGGER from nav.config) so the module uses the same public API as
backplanes.py and other files; update the import statement referencing
IMAGE_LOGGER accordingly.
In `@src/main/nav_create_simulated_image.py`:
- Around line 1201-1202: The code currently stashes dynamic attributes like
w.center_v_spin and w.center_u_spin onto a QWidget with # type:
ignore[attr-defined], which is fragile; replace these ad-hoc attributes by
defining small dataclasses (e.g., BodyTabWidgets, RingTabWidgets) or TypedDicts
that list the widget references and have the tab-build function (the function
that constructs the tab and currently sets w.* attributes) return an instance of
that dataclass; then store that dataclass instance in a parallel dict keyed by
tab index (e.g., widgets_by_tab[int] = BodyTabWidgets(...)) and access widgets
via that typed structure instead of w.<attr>, removing the # type:
ignore[attr-defined] usages (including occurrences around
center_v_spin/center_u_spin and the other lines noted).
In `@src/main/nav_mosaic_display.py`:
- Around line 151-164: The top-level help flags are being swallowed by the mode
dispatch in main(); before checking args_list[0] you should intercept '-h' or
'--help' and show top-level usage. Modify main() to check if '-h' or '--help'
appears in args_list, and if so create an
argparse.ArgumentParser(prog='nav_mosaic_display'), call its print_help() (or
error/help output as appropriate) and sys.exit(0) so users see the global help;
keep the rest of the flow using _build_parser(mode) and dispatching to
_run_rings/_run_body unchanged.
In `@src/main/nav_mosaic.py`:
- Around line 97-98: The module-level globals DATASET and DATASET_NAME are set
in parse_args and then read elsewhere; instead have parse_args return (dataset,
dataset_name) and thread those values through callers (main -> _run_body and
_run_rings) so those functions accept dataset and dataset_name parameters rather
than reading globals; remove the global assignments, the `global` statements and
the assert/# type: ignore[arg-type] lines in _run_body/_run_rings (and callers)
and update all call sites to pass the returned dataset and dataset_name so mypy
can narrow types and the code works safely if main is invoked multiple times.
- Around line 182-239: Extract a shared helper (e.g., _run_reproject_pass) to
consolidate the duplicated loop logic in nav_mosaic.py: move the per-image
output path computation (per_image_output_path), overwrite/skip/dry-run checks,
IMAGE_LOGGER.open scaffolding created by _reproject_image_log_handlers, loading
of obs via obs_class.from_file, applying offsets via load_offset_if_any and
apply_offset_to_obs, saving results, and exception handling via
_log_main_exception into that helper; have the helper accept a reprojection
callable (reproject_fn) which will be called with the obs and image_name so
callers can pass reproject_one_body or a lambda wrapping reproject_one_ring with
args, and keep log label and final counting (n_done/n_skipped) returned to the
caller so both the "body" and "rings" branches simply call
_run_reproject_pass(...) with the appropriate reproject_fn and label.
- Around line 206-237: The try/except around the per-image block (inside
IMAGE_LOGGER.open) is too broad; replace the bare "except Exception:" with
targeted handlers: catch expected recoverable errors (e.g., IOError/OSError,
ValueError, your custom parsing errors) and handle them by calling
_log_main_exception with the caught exception instance (pass the exception to
preserve traceback), but explicitly re-raise KeyboardInterrupt and SystemExit
(or use "except Exception as e: if isinstance(e, (KeyboardInterrupt,
SystemExit)): raise") so the process can abort; for CI/fail-fast behavior add a
check for args.strict (or --fail-fast) and re-raise the caught exceptions when
that flag is set; narrow the try to the smallest failing statements (e.g., wrap
load_offset_if_any/apply_offset_to_obs, reproject_one_body, and result.save each
in their own try/except) and reference the existing symbols (IMAGE_LOGGER.open,
load_offset_if_any, apply_offset_to_obs, reproject_one_body, result.save,
_log_main_exception, args.no_write_output_files, args.image_name) when making
these changes.
In `@src/nav/dataset/dataset_pds3.py`:
- Around line 592-599: The loop in _yield_image_files_index calls
_get_img_name_from_label_filespec which can raise ValueError and currently will
abort iteration; wrap the call to _get_img_name_from_label_filespec inside a
try/except ValueError, log a warning that includes the offending
explicit_img_filespec and the exception, then continue (skip) that
entry—mirroring the existing None-skip behavior—so build
new_img_name_filter_list only from successfully resolved names.
In `@src/nav/reproj/_serialization.py`:
- Around line 311-356: The np.load(local_path, allow_pickle=False) call returns
an NpzFile that must be closed to avoid leaking file descriptors; wrap the call
in a context manager (with np.load(local_path, allow_pickle=False) as raw:) and
move all subsequent uses of raw (kind/version/keys/array access) inside that
with-block so the archive is closed when done; keep the existing handling logic
for '__kind__', '__version__', MaskedArray reconstruction (ma_keys loop), None
sentinels (none_keys loop), and the final scalar/array unwrapping unchanged,
using the same variable names (local_path, raw, keys, handled, result).
In `@src/nav/reproj/bodies.py`:
- Around line 1824-1848: The _validate_repro method in BodyMosaic currently
omits a photometric model check allowing corrected and uncorrected reprojections
to be mixed; update _validate_repro (in class BodyMosaic) to compare
repro.photometric_model_name against the mosaic's stored photometric model
(self._photometric_model_name) and raise a ValueError on mismatch (use a clear
message like 'photometric_model_name mismatch: mosaic=..., repro=...'),
mirroring the behavior enforced in RingMosaic.add() and referencing the
BodyReprojResult.photometric_model_name attribute.
- Around line 1200-1206: The code adds u_min/v_min to good_u/good_v before
indexing ok_body_mask, but good_u/good_v are subimage-relative and ok_body_mask
may already be subimage-sized when override_backplane is used; make the
coordinate system consistent before masking by checking whether ok_body_mask is
in full-frame or subimage coordinates (or use the override_backplane flag) and
only add u_min/v_min when ok_body_mask is full-frame; otherwise index
ok_body_mask with good_u and good_v directly so ok_on_detector =
ok_body_mask[good_v, good_u] for subimage backplanes and ok_on_detector =
ok_body_mask[good_v + v_min, good_u + u_min] for full-frame backplanes (refer to
ok_body_mask, good_u, good_v, u_min, v_min, subimg, override_backplane).
- Around line 830-909: The fast-path for off-detector frames still allocates
full-planet buffers; change allocation to create only the tiny placeholder
window instead of full-size arrays. Compute sub_shape = (max_lat_pixel -
min_lat_pixel + 1, max_lon_pixel - min_lon_pixel + 1) and call
_create_repro_array with those dimensions (use sub_shape[0], sub_shape[1],
dtype) for
repro_img_full/repro_res_full/repro_phase_full/repro_emission_full/repro_incidence_full
(or avoid the “_full” naming and allocate only the sub arrays), and adjust
eff_res creation to operate on that small array (respecting
navigation_uncertainty) before returning the BodyReprojResult in the
mask_only/off-detector path.
- Around line 674-676: The current computation of self._n_full_lat and
self._n_full_lon uses truncation and misses the final bin; change them to match
the floor-plus-one logic used by generate_latitudes() and generate_longitudes()
so the end bins are included — i.e., compute the counts as floor(pi /
lat_resolution) + 1 for self._n_full_lat and floor(2*pi / lon_resolution) + 1
for self._n_full_lon (using the same lat_resolution/lon_resolution and
math.floor or equivalent).
In `@src/nav/reproj/cartographic_model.py`:
- Around line 115-137: bp_latitude/ bp_longitude -> row_coords/col_coords are
MaskedArrays and you must fill their masked entries before building coords so
map_coordinates never sees NaN; change the code that computes row_coords and
col_coords to call .filled(...) (or otherwise convert to a plain ndarray) before
the np.array([...]) stack (i.e., ensure row_coords.ravel() and
col_coords.ravel() are finite floats), then pass those filled arrays to
map_coordinates; keep the existing in_bounds, mosaic_fill, and model_img masking
logic as-is.
In `@src/nav/reproj/ring_orbit_model.py`:
- Around line 108-110: The sign-convention TODO on _longitude_shift must be
resolved: add a unit test that verifies the current formula in _longitude_shift
(which uses mean_motion and _epoch_et) by performing a round-trip assert that
corotating_to_inertial(inertial_to_corotating(L, et), et) == L for multiple et
values far from _epoch_et and also compare the computed shift against a known
reference value from the legacy implementation (or FRING_CORE) to ensure
absolute correctness; if the round-trip or reference comparison fails, flip the
sign in the expression in _longitude_shift (negate the numerator or remove the
leading minus) and re-run tests until both the round-trip and reference-value
checks pass, then remove the TODO comment.
In `@src/nav/reproj/rings.py`:
- Around line 1116-1120: The shadow mask is applied to image data but not to the
geometry backplanes, so functions like repro_mean_phase, repro_mean_emission,
mean_radial_resolution, and repro_incidence still include shadowed pixels;
update the code in the omit_shadow branch (where omit_shadow, shadow, and data
are defined) to also apply the same shadow mask to the geometry/backplane arrays
(or fold shadow into the existing good mask) so that those backplanes are masked
consistently (e.g., mask the backplane arrays and/or combine shadow with good
before any per-column averaging).
- Around line 1465-1468: The code in add() currently sets self._mean_incidence =
repro.incidence, which overwrites previous values; change to maintain a running
sum and compute the mean so all images contribute: add a field (e.g.
self._mean_incidence_sum) and update it in add() using self._mean_incidence_sum
+= repro.incidence, increment self._image_count (already updated), and then
recompute self._mean_incidence = self._mean_incidence_sum / self._image_count
(or implement mean_incidence as a property that divides the stored sum by
_image_count). Update any initialization and serialization accordingly so
RingMosaicData.mean_incidence reflects the true aggregate.
- Around line 1393-1438: The add() method must validate that repro
(RingReprojResult) is compatible before merging: check repro.body equals the
mosaic's body (self._body or self._grid.body), verify repro.grid resolution and
bounds match the mosaic grid (compare radius_step/radius_spacing,
longitude_step/num_longitude or equivalent attributes on repro.grid vs
self._grid, and radius min/max/bounds), and ensure the repro sparse array dtypes
(repro.longitude.dtype and repro.radius.dtype or the repro.dtype_pair) match the
mosaic's expected dtypes (the mosaic's stored dtype pair or arrays). For any
mismatch raise a clear ValueError naming the field (e.g., "body", "grid
resolution", "radius bounds", "dtype") and the mosaic vs repro values; perform
these checks early in RingMosaic.add() before orbit/photometric checks or any
array insertion.
- Around line 715-716: The value of _n_full_lon is computed with int(2π /
longitude_resolution) which truncates and ends up one bin short; change the
calculation in the constructor so _n_full_lon equals floor(2π /
longitude_resolution) plus one (e.g., _n_full_lon = int(math.floor(2.0 * math.pi
/ longitude_resolution)) + 1) so the module's inclusive indexing (0 through
floor(_MAX_LONGITUDE/longitude_resolution)) is representable and avoids
out-of-range indexing in code that uses _n_full_lon and longitude_resolution.
In `@src/nav/ui/__init__.py`:
- Around line 1-18: Add an explicit public API declaration by adding an __all__
variable to the package __init__.py (the module currently only contains the
module docstring). Modify the top-level of src/nav/ui/__init__.py to define
__all__ = [] (an empty list is fine since nothing is re-exported) so the package
follows the project's __init__.py policy and clearly declares its public
surface.
In `@src/nav/ui/common.py`:
- Around line 23-52: apply_linear_gamma_stretch currently silently clamps
invalid inputs (white <= black and gamma <= 0) and uses a hardcoded 1e-6; change
this to explicit validation consistent with build_stretch_controls() and
_require_finite_int_or_float: validate that black and white are finite numbers
and that white > black (raise ValueError) and validate gamma is finite and > 0
(raise ValueError or TypeError as appropriate), remove the silent nudge, and
introduce a documented module-level constant (e.g. MIN_RANGE_EPS) instead of
hardcoded magic numbers if you still need a small epsilon elsewhere; update
tests in test_common.py to expect the new exceptions.
In `@src/nav/ui/mosaic_viewer/__init__.py`:
- Around line 1-9: Add an explicit __all__ declaration to this package's
__init__.py to document the public API surface; since the package currently
re-exports nothing, add a module-level __all__ = [] (i.e., define the symbol
__all__ in __init__.py) so that from nav.ui.mosaic_viewer import * is explicit
and matches the project's init.py policy.
In `@src/nav/ui/mosaic_viewer/body_window.py`:
- Around line 214-1212: The BodyMosaicWindow class is >1000 lines and mixes many
responsibilities; split it into focused modules and have BodyMosaicWindow
compose them. Extract file-loading logic (load_body_file usage and _load_file)
into a FileLoader helper, move stretch/zoom UI and logic (_build_control_panel,
_apply_stretch, _fit_zoom_to_window, _sync_zoom_ui, _make_zoom_sync) into a
ZoomStretchController, pull overlays/tick computations (_update_overlays,
_update_axis_ticks, _nice_overlay_step) into an Overlays module, put
photometry/image generation (_apply_body_display_image, _photometry_mode,
_sync_photometry_ui, _on_photometry_changed, compute_body_display_image call)
into a Photometry service, move color-by logic (_compute_color_tint,
_tint_with_alpha, _update_colorby_widgets_for_mosaic, _on_colorby_changed,
_on_colorby_alpha_changed) into a ColorBy component, and isolate cursor readout
(_on_mouse_moved, _clear_info, _fmt_ma, _fmt_deg_label) into a CursorInfo
helper; create small modules (e.g. file_loader, zoom_stretch, overlays,
photometry, colorby, cursor_info), update BodyMosaicWindow to instantiate and
delegate to these helpers (keeping UI wiring in
_setup_ui/_build_right_panel/_build_control_panel but delegating behavior), and
adjust imports/signals so unit tests can target each helper independently.
- Around line 95-125: The helper _colorby_tint can raise in
np.nanmin()/np.nanmax() when the input has no finite (all-masked or all-NaN)
values; detect that case early and return a neutral tint instead. Add a check
after arr is prepared (use np.isfinite(arr).any() or np.isfinite(arr).any() ==
False) and if there are no finite values, construct and return an array of shape
(n_rows, n_cols, 3) filled with 0.5 of dtype float32 (matching the existing
neutral tint behavior) rather than calling np.nanmin/np.nanmax; keep the rest of
the logic unchanged for normal data. Ensure you reference _colorby_tint, arr,
and the rgb neutral value 0.5 when making the change.
In `@src/nav/ui/mosaic_viewer/common.py`:
- Line 231: The assignment uses falsy checks that conflate empty values with
missing ones; change the ternaries to explicit None checks so presence is
determined by identity, e.g., replace "orbit_model_name =
result.orbit_model.name if result.orbit_model else None" with an explicit check
of result.orbit_model is not None and likewise replace any "if orbit_model_name
else None" usages (also at the other occurrences you noted around lines 288-289)
with "is not None" checks so an empty string is preserved as present while only
a true None becomes missing; update the code paths that set or use
orbit_model_name and result.orbit_model accordingly.
- Around line 433-437: Fix the latitude reconstruction by applying the required
-π/2 offset for BodyReprojResult index-to-radian conversion and remove the stray
parentheses around the upper index multiplications; specifically, compute
lat_min and lat_max as (result.lat_idx_range[0] * result.lat_resolution -
math.pi/2) * _RAD_TO_DEG and (result.lat_idx_range[1] * result.lat_resolution -
math.pi/2) * _RAD_TO_DEG, and compute lon_min and lon_max as
result.lon_idx_range[0] * result.lon_resolution * _RAD_TO_DEG and
result.lon_idx_range[1] * result.lon_resolution * _RAD_TO_DEG (use the existing
symbols result.lat_idx_range, result.lat_resolution, result.lon_idx_range,
result.lon_resolution, and _RAD_TO_DEG; mirror the radian semantics used by
BodyMosaicData).
In `@src/nav/ui/mosaic_viewer/graticule.py`:
- Around line 112-121: The check in graticule_label_anchors should mirror
graticule_polylines by raising TypeError when params is None and the docstring
Raises: section should reflect that change; update the params precondition from
raising ValueError to raising TypeError (keeping the existing non-negative check
for lat_step_deg and lon_step_deg as ValueError) and modify the docstring to
list TypeError for the params-is-None case and ValueError for negative step
arguments so both functions are consistent.
In `@src/nav/ui/mosaic_viewer/matplotlib_qt.py`:
- Around line 23-31: The function canvas_draw_idle has a broad annotation of
FigureCanvasQTAgg | Any which collapses to Any; change the parameter type to
FigureCanvasQTAgg | object (or simply object) so callers must narrow before
using canvas-specific APIs, update imports to drop typing.Any if removed, and
remove the redundant "Returns:" stanza from the docstring; keep the docstring
describing the parameter and behavior and ensure the signature and imports
reference FigureCanvasQTAgg and the draw_idle method.
In `@src/nav/ui/mosaic_viewer/ring_window.py`:
- Around line 1521-1532: _in_mouse_moved currently derives ix from round(px)
which breaks under pan/zoom; change it to map the widget X to array column
instead of using raw px: call the widget's X-mapping (e.g.
image_widget.pixel_x_to_arr_col(px) if present) to get the array column index,
or if that helper doesn't exist use image_widget.pixel_to_physical(px, py) to
get lon_deg and convert that longitude to a column index against the display
data's longitude grid (dd.longitude) using np.searchsorted/closest index, then
clip to [0, dd.n_longitude-1], set _last_profile_lon_ix to that int, and use
that index for img[arr_row, ix] so sampled values and metadata stay aligned with
the cursor.
- Around line 284-1664: The RingMosaicWindow class is too large and mixes
responsibilities (plotting, EW/radial profile logic, color-by, file
loading/navigation, and UI construction); split it into smaller
controller/helper modules to keep files under 1000 lines. Create separate
modules/classes (e.g., EwPanelController handling
_ew_*/_replot_corot_ew_panel/_draw_corot_band_curve/_add_ew_range_to_plot/_sync_ew_*/_init_corot_ew_axes
and state like _ew_data/_ew_mu_data/_ew_radial_ranges/_ew_phase;
RadialProfileController handling
_radial_*/_update_radial_profile_plot/_init_radial_axes/_safe_radial_canvas_draw
and _radial_profile_line state; ColorByController handling
_on_colorby_changed/_compute_color_column/_tint_with_alpha/_on_colorby_alpha_changed
and _colorby_alpha state; and FileNavigation/FileLoad helper for
_load_file/_populate_file_list/_on_prev/_on_next/_refresh_file_list_selection),
move the corresponding methods and state into those modules, update
RingMosaicWindow to instantiate and delegate to them (keeping only high-level UI
wiring like _setup_ui, signals, and calls to controller APIs such as
apply_display_image, recompute_ew, update_show_radii), and adjust
imports/attributes accordingly so behavior is unchanged while RingMosaicWindow
becomes a thin coordinator.
In `@src/nav/ui/mosaic_viewer/sphere_render.py`:
- Around line 98-109: Update the function/method docstring for parameters
lat_deg and lon_deg (referencing variables like lat_deg, lat_min_deg, d_lat_deg,
n_data_rows and lon_bin_to_dc) to explicitly state that latitudes outside the
interval [lat_min_deg, lat_min_deg + n_data_rows * d_lat_deg) are treated as
off-grid and rendered as no-data, and that longitudes mapped by lon_bin_to_dc ==
-1 are treated as off-grid/no-data; keep the existing range note (lat in
[-90,90]) and add one concise sentence for each parameter describing this
off-grid/no-data behavior so the code behavior shown around lon_r, k, dc and dr
is documented.
- Around line 162-171: The QImage constructor used (QImage(bytes, w, h,
bytesPerLine, format)) stores a pointer to the supplied buffer so the temporary
`rgb_c.tobytes()` and local `rgb_c` go out of scope and lead to use-after-free;
fix by creating a persistent Python buffer (e.g., produce a bytearray or
memoryview from the contiguous numpy array created in this function `rgb_c =
np.ascontiguousarray(...)`), pass that persistent buffer into the QImage
constructor, then attach that buffer to the returned QImage object (e.g., set an
attribute like `qimg._buf = buffer`) so the buffer remains alive for the QImage
lifetime before returning the QImage.
In `@src/nav/ui/mosaic_viewer/tiled_image_widget.py`:
- Around line 662-690: set_projection currently updates _proj_scale (for
non-RECT) and swaps the active zoom model (for RECT) but never notifies
listeners; at the end of set_projection (after self.viewport().update()) compute
the current zoom (use self._proj_scale when kind != ProjectionKind.RECT,
otherwise read the active/rect zoom model value e.g. self.zoom_model.value() or
self.active_zoom_model.current_value) and emit the zoom_changed signal with that
value (self.zoom_changed.emit(current_zoom)) so UI sliders/state stay in sync.
In `@src/reproj_cli/offsets.py`:
- Around line 80-98: The code calls nav_metadata.get('status') after casting
json.loads(text) to dict without validating the JSON root; add an explicit type
check: after nav_metadata = json.loads(text) verify isinstance(nav_metadata,
dict) (or dict[str, Any]) and if not, call MAIN_LOGGER.warning with the same
message pattern referencing image_file.image_file_url and the unexpected
type/value, then return None; only after that check use
nav_metadata.get('status') and proceed as before (remove or relocate the cast to
after the isinstance guard).
In `@src/reproj_cli/paths.py`:
- Around line 96-111: The docstrings for per_image_output_path and
mosaic_output_path contain an unbalanced backtick for the file format `'npz'`
(rendered as ``'npz``'``), breaking inline code formatting; update both
docstrings to use balanced backticks around 'npz' (e.g., `'npz'` or ``'npz'``
consistently) so the inline code renders correctly and matches the existing
`'fits'` formatting, leaving all other text unchanged.
- Around line 33-54: The function _subject_filename_segment performs redundant
sanitization: the initial regex substitution using _INVALID_FILENAME_CHARS plus
space replacement and collapse is superseded by the later character-by-character
rebuild that already replaces any non-alnum/._- with '_'. Simplify by trimming
subject_name once (subject_name.strip()), then perform a single pass that maps
every invalid character to '_' (the existing comprehension used to produce out),
collapse consecutive underscores (e.g. while '__' in out: out =
out.replace('__','_')), strip leading/trailing '_' and finally return 'unknown'
if empty; remove the earlier _INVALID_FILENAME_CHARS.sub, the separate space
replacement, and the first collapse/strip so only one sanitization pass remains.
- Around line 71-85: The function _ensure_output_under_dir currently coerces any
FCPath URI to a local Path; change it to detect remote URIs up-front and reject
them with a clear ValueError (e.g., construct fc = FCPath(output_dir); if fc has
a non-empty scheme like "gs","s3","file" then raise ValueError("remote
output_dir not supported: ...")). Update related places (per_image_output_path
and mosaic_output_path docstrings/signatures) to document that output_dir must
be a local path or alternatively implement cloud-aware normalization using
FCPath without converting to pathlib — but do not silently convert URIs to local
filesystem paths in _ensure_output_under_dir.
In `@tests/nav/reproj/test_bodies.py`:
- Around line 684-692: The new test currently only covers the BodyMosaicData
path; add a minimal round-trip test that saves a BodyReprojResult and calls
load_body_file to exercise the BodyReprojResult branch in load_body_file and
verify lat_range_deg/lon_range_deg conversion. Create a test (e.g.,
test_load_body_file_reproj_result_extent) that builds a repro via
_make_repro(lat_range=(5,7), lon_range=(10,12), image_name='x'), saves it with
repro.save(path, format='fits'), calls dd = load_body_file(str(path)), and
asserts dd.lat_range_deg[0] (and/or lat_range_deg[1]/lon_range_deg) equals the
expected value computed using the same conversion used in common.py (include the
-π/2 offset) with pytest.approx for floating comparison; this will cover the
BodyReprojResult branch and catch the missing offset.
---
Outside diff comments:
In `@src/nav/dataset/dataset_pds3.py`:
- Around line 592-735: The img_name_filter_list from
yield_image_files_from_arguments is not normalized to the same case as
index-derived img_name, so membership checks in the loop (img_name_filter_list
vs img_name in _get_img_name_from_label_filespec) can silently miss matches;
change resolution in yield_image_files_from_arguments to normalize each
CSV-resolved filespec via _get_img_name_from_label_filespec(...).upper() (or
otherwise call .upper() on the returned name) and store them in a set
(img_name_filter_list -> img_name_filter_set) to both normalize casing and make
membership O(1), then in this loop compare img_name.upper() against that set (or
ensure both sides are uppercased) when checking inclusion.
In `@src/nav/support/correlate.py`:
- Around line 582-591: The no-candidate fallback in correlate.py returns a dict
missing the 'peak_val' key, causing navigate_with_pyramid_kpeaks() (which reads
res_lvl["peak_val"] unconditionally) to raise KeyError; update the fallback
returned by the candidates-empty branch to include a 'peak_val' entry (set to a
sensible low/zero value consistent with existing semantics, e.g., 0.0 or
-np.inf) alongside 'offset', 'cov', 'sigma_xy', and 'quality' so res_lvl always
contains the same keys expected by navigate_with_pyramid_kpeaks() and downstream
code.
- Around line 404-421: The subpixel refinement is using the raw numerator
spectrum `spec` (from image_pad/model_pad/mask_pad) which reintroduces bias;
instead use the normalized NCC surface (`corr`) as the input to `upsampled_dft`
so refinement is performed on the same normalized peak `nms_topk()` selected.
Replace the `spec = fft2(image_pad) * np.conj(fft2(model_pad * mask_pad))` usage
with a padded FFT of the normalized correlation (`corr`) region (e.g., build a
padded `corr_pad` matching the current padding and call `spec =
fft2(corr_pad)`), then pass that `spec` into `upsampled_dft(spec,
upsample_factor, (region_v, region_u), (oy - dy_i * upsample_factor, ox - dx_i *
upsample_factor))` so `Up`/`upy, upx` are computed from the normalized surface
rather than the unnormalized numerator.
In `@src/nav/support/image.py`:
- Around line 256-271: array_zoom currently omits input validation and has an
outdated docstring; update array_zoom's docstring to describe the
np.repeat-based implementation, require integer zoom factors >=1, and document
the Raises behavior, then add explicit checks at the function start that a.ndim
== len(factor) (mirror shift_array/pad_array/unpad_array), validate that each
element of factor is an int and >= 1 (raise TypeError for non-int, ValueError
for <1), and raise a clear ValueError if lengths mismatch so callers get
deterministic, helpful errors rather than AxisError or silent no-ops.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0df7cf46-d0c0-4645-b25b-296e2012400d
📒 Files selected for processing (97)
.readthedocs.yamlCLAUDE.mdCODEBASE_ANALYSIS.mdREADME.mddocs/Makefiledocs/api_reference.rstdocs/api_reference/api_reproj.rstdocs/api_reference/api_ui.rstdocs/developer_guide.rstdocs/developer_guide_backplanes.rstdocs/developer_guide_best_practices.rstdocs/developer_guide_building_docs.rstdocs/developer_guide_class_hierarchy.rstdocs/developer_guide_configuration.rstdocs/developer_guide_nav_technique_correlate.rstdocs/developer_guide_navigation_models_rings.rstdocs/developer_guide_reprojection.rstdocs/user_guide.rstdocs/user_guide_backplanes.rstdocs/user_guide_navigation.rstdocs/user_guide_pds4_bundle.rstdocs/user_guide_reprojection.rstdocs/user_guide_simulated_images.rstpyproject.tomlsrc/backplanes/backplanes.pysrc/backplanes/writer.pysrc/experiments/fov_twist/find_fov_twist.pysrc/main/nav_backplane_viewer.pysrc/main/nav_backplanes.pysrc/main/nav_create_bundle.pysrc/main/nav_create_bundle_cloud_tasks.pysrc/main/nav_create_simulated_image.pysrc/main/nav_mosaic.pysrc/main/nav_mosaic_display.pysrc/nav/config/__init__.pysrc/nav/config/config.pysrc/nav/config/logger.pysrc/nav/dataset/dataset.pysrc/nav/dataset/dataset_pds3.pysrc/nav/dataset/dataset_pds3_cassini_iss.pysrc/nav/nav_model/rings/ring_feature.pysrc/nav/nav_technique/nav_technique_correlate_all.pysrc/nav/obs/obs_inst_cassini_iss.pysrc/nav/obs/obs_inst_galileo_ssi.pysrc/nav/obs/obs_inst_newhorizons_lorri.pysrc/nav/obs/obs_inst_sim.pysrc/nav/obs/obs_inst_voyager_iss.pysrc/nav/obs/obs_snapshot.pysrc/nav/reproj/__init__.pysrc/nav/reproj/_context_managers.pysrc/nav/reproj/_serialization.pysrc/nav/reproj/bodies.pysrc/nav/reproj/cartographic_model.pysrc/nav/reproj/photometric_model.pysrc/nav/reproj/ring_orbit_model.pysrc/nav/reproj/rings.pysrc/nav/support/__init__.pysrc/nav/support/correlate.pysrc/nav/support/correlate_old.pysrc/nav/support/image.pysrc/nav/support/misc.pysrc/nav/support/nav_base.pysrc/nav/ui/__init__.pysrc/nav/ui/common.pysrc/nav/ui/manual_nav_dialog.pysrc/nav/ui/mosaic_viewer/__init__.pysrc/nav/ui/mosaic_viewer/body_window.pysrc/nav/ui/mosaic_viewer/common.pysrc/nav/ui/mosaic_viewer/graticule.pysrc/nav/ui/mosaic_viewer/matplotlib_qt.pysrc/nav/ui/mosaic_viewer/photometric_display.pysrc/nav/ui/mosaic_viewer/projections.pysrc/nav/ui/mosaic_viewer/ring_window.pysrc/nav/ui/mosaic_viewer/sphere_render.pysrc/nav/ui/mosaic_viewer/tiled_image_widget.pysrc/reproj_cli/__init__.pysrc/reproj_cli/args.pysrc/reproj_cli/factories.pysrc/reproj_cli/offsets.pysrc/reproj_cli/paths.pysrc/reproj_cli/reproject.pytests/conftest.pytests/nav/nav_model/test_nav_model_rings.pytests/nav/nav_model/test_nav_model_rings_simulated.pytests/nav/reproj/__init__.pytests/nav/reproj/test_bodies.pytests/nav/reproj/test_cartographic_model.pytests/nav/reproj/test_rings.pytests/nav/reproj/test_serialization_fits.pytests/nav/reproj/test_shared_modules.pytests/nav/support/test_upsampled_dft.pytests/nav/ui/test_common.pytests/nav/ui/test_graticule.pytests/nav/ui/test_projections.pytests/nav/ui/test_sphere_render.pytests/reproj_cli/test_factories.pytests/reproj_cli/test_paths.py
💤 Files with no reviewable changes (3)
- src/nav/dataset/dataset.py
- src/nav/config/init.py
- src/nav/support/correlate_old.py
Comment on lines
+1
to
+188
| ## 1. Structure and layout | ||
|
|
||
| - **High — Top-level package names collide with the Python ecosystem.** | ||
| `src/main`, `src/backplanes`, `src/pds4`, `src/reproj_cli`, and (as a | ||
| namespace package) `src/util` are installed as top-level importable | ||
| names alongside `nav`. `pyproject.toml` lines 186-187 | ||
| (`[tool.setuptools.packages.find] where = ["src"]`) discovers them all; | ||
| `[project.scripts]` lines 197-210 use them as `main.nav_offset:main`, | ||
| `reproj_cli.paths`, etc. Anyone else on PyPI (or in a larger app) using | ||
| a top-level `main` / `util` / `backplanes` will silently have their | ||
| imports shadowed. **Suggestion:** move them under `nav` (e.g. | ||
| `nav._main`, `nav._cli`, `nav.backplanes`, `nav.pds4`). The | ||
| `reproj_cli` package already has a CLAUDE.md comment ("this package is | ||
| not part of the importable `nav` public API") that hints at this | ||
| intent; finish the job. | ||
|
|
||
| ## 4. Testing | ||
|
|
||
| **Config.** `pyproject.toml [tool.pytest.ini_options]` only sets | ||
| `pythonpath = ["src"]`. No `addopts`, no `-n auto`, no `--dist=loadfile` | ||
| — so plain `pytest` does a serial run. CLAUDE.md tells developers to | ||
| invoke `pytest -n auto --dist=loadfile` manually, and `scripts/run-all- | ||
| checks.sh` line 249 uses `-n auto` (missing the `--dist=loadfile`!) and | ||
| the CI workflow at `.github/workflows/run-tests.yml` line 91 uses both. | ||
| **Suggestion:** put both flags in `pyproject.toml` so all invocations | ||
| are consistent. | ||
|
|
||
| - **Critical — `.coveragerc` scopes coverage to `nav` only.** | ||
| `.coveragerc`: `source = nav`. This excludes `backplanes/`, `main/`, | ||
| `pds4/`, `reproj_cli/`, `util/` — i.e. all the CLI entry points, the | ||
| backplanes generator, the PDS4 bundle generator, and the reproj CLI | ||
| helpers. The PBP target of 90% coverage is therefore misleadingly | ||
| easy. **Suggestion:** `source = nav, backplanes, pds4, reproj_cli` | ||
| (leave `main/` out if you also leave entry-point scripts untested). | ||
| The `[run]` section should also set `parallel = True` if tests ever | ||
| run under xdist with coverage. | ||
|
|
||
| - **Critical — `codecov.yml` disables coverage enforcement.** | ||
| Both `coverage.status.project.default.target` and `patch.default.target` | ||
| are set to `0` — i.e., codecov will never block a PR on coverage drop. | ||
| If the intent is "informational only", the file is correct; if | ||
| enforcement is wanted, set realistic thresholds. | ||
|
|
||
| - **High — `pytest.raises` without message content (PBP §7 | ||
| "ALWAYS...assert on the exception message content").** | ||
| Several tests use `pytest.raises(Exception)` without `match=`. Examples: | ||
| - `tests/nav/support/test_misc.py` line 16: `with pytest.raises(ValueError):` | ||
| - `tests/nav/dataset/test_dataset_pds3_cassini_iss.py` line 130. | ||
| With `pytest.raises(ValueError, match=...)` you guard against the | ||
| wrong ValueError being raised and accidentally "passing". | ||
|
|
||
| - **High — Most `tests/nav/inst/*` tests are 8-line smoke tests.** | ||
| `tests/nav/inst/test_inst_cassini_iss.py`, `_galileo_ssi.py`, | ||
| `_newhorizons_lorri.py`, `_voyager_iss.py` are each 8 lines and assert | ||
| a single magic number (e.g. `obs.midtime == 196177280.54761`). They | ||
| depend on live PDS holdings. **Suggestion:** add a coverage test for | ||
| `get_public_metadata()`, `star_min_usable_vmag`, `star_max_usable_vmag` | ||
| for each instrument. | ||
|
|
||
| - **High — Tests in `tests/nav/dataset/` depend on remote PDS index | ||
| counts.** | ||
| `tests/nav/dataset/test_dataset_pds3_cassini_iss.py` line 54 asserts | ||
| `len(ret) == 8868` on a volume index. This is fragile against remote | ||
| index updates. Documenting the frozen expectation is fine; the | ||
| assertion should at least check a lower bound (`>= 8868`) or be | ||
| parametrised with the expected volume cut. | ||
|
|
||
| - **Medium — Ring-model tests use `MagicMock` throughout.** | ||
| `tests/nav/nav_model/test_nav_model_rings.py` is a heavy `MagicMock` | ||
| suite (>1000 lines, 25+ tests). Good coverage of the orchestrator, but | ||
| many tests set up mocks three pages deep — a small helper or fixture | ||
| factory in `conftest.py` would cut duplication. | ||
|
|
||
| - **Medium — `tests/__init__.py` and several other `__init__.py` files | ||
| are empty.** | ||
| `tests/__init__.py`, `tests/nav/ui/__init__.py`, `tests/nav/reproj/__init__.py`. | ||
| Empty namespaces are fine; but when test imports use | ||
| `from tests.config import URL_…`, the loader depends on the empty | ||
| `__init__.py` existing. Better: either make them proper packages with | ||
| a docstring, or remove them and rely on pytest's implicit rootdir. | ||
|
|
||
| - **Medium — Test suite largely skips UI rendering.** | ||
| `tests/nav/ui/test_common.py` starts with three layered `pytest.skip` | ||
| guards (lines 14-37) — if PyQt6 / EGL / `nav.ui.common` import fails, | ||
| the whole module is skipped. In CI this is fine (EGL is installed per | ||
| `.github/workflows/run-tests.yml` line 73). For maintenance, document | ||
| this skip path. | ||
|
|
||
| ## 8. Dependencies and tooling | ||
|
|
||
| - **High — CI publishing workflows are inconsistent with each other | ||
| and with the project rulebook.** | ||
| - `.github/workflows/publish_to_pypi.yml` uses `actions/checkout@v4`, | ||
| `actions/setup-python@v5`; while `run-tests.yml` uses `@v6` | ||
| (`environment_best_practices.mdc` line 17 pins to a major tag — so | ||
| different major tags across workflows is a minor inconsistency). | ||
| - The PyPI publish workflow uses a `PYPI_API_TOKEN` secret rather than | ||
| PEP 740 Trusted Publishers (also called for in the rulebook under | ||
| "Publishing workflow … Trusted Publishers or token auth"). Token | ||
| auth works but Trusted Publishers is recommended. | ||
| - No `pip audit` job — `dependency_management.mdc` §5 says "Run `pip | ||
| audit` in CI". | ||
| - No Dependabot or Renovate configuration (nothing under `.github/` | ||
| matches); also explicitly called out by the rulebook. | ||
|
|
||
| - **Medium — Separate `.coveragerc` when `pyproject.toml` supports | ||
| `[tool.coverage.run]`.** | ||
| `dependency_management.mdc` §6 explicitly says "Do NOT create separate | ||
| config files (`.coveragerc`…) when the tool supports `pyproject.toml`". | ||
|
|
||
| - **Medium — `requirements.txt` shape is non-standard.** | ||
| `-e .\n-e .[dev]`. `dependency_management.mdc` §1 says "If a | ||
| `requirements.txt` is kept, it should contain only `-e .` for backward | ||
| compatibility." Having `-e .[dev]` in `requirements.txt` means | ||
| anyone running `pip install -r requirements.txt` installs dev deps | ||
| — surprising. Suggest just `-e .[dev]` and a comment pointing users | ||
| at `pip install -e ".[dev]"` as the preferred command. | ||
|
|
||
| - **Medium — Python matrix in CI is broader than `pyproject.toml` | ||
| "Development Status".** | ||
| `pyproject.toml` line 35 says `Development Status :: 2 - Pre-Alpha` | ||
| while CI tests across Python 3.10-3.13 and the README carries PyPI | ||
| badges (Release, Downloads). If you're publishing to PyPI, `2 - | ||
| Pre-Alpha` is unusual. | ||
|
|
||
| - **Low — `scripts/run-all-checks.sh` pytest command omits | ||
| `--dist=loadfile`.** | ||
| Line 249: `python -m pytest tests -q --cov -n auto`. CLAUDE.md calls | ||
| this out: "pytest-xdist must run with `--dist=loadfile`; default | ||
| scheduling crashes PyQt6 workers". Add the flag or point the script | ||
| at the CI command. | ||
|
|
||
| - **Low — `pyinstrument` and `pytest-profiling` in dev deps but no | ||
| docs on how to use them.** | ||
| Not a correctness issue, but "why is this installed" is a question | ||
| that comes up. | ||
|
|
||
| --- | ||
|
|
||
| ## 9. Technical debt and risk | ||
|
|
||
| - **High — 80+ `TODO`/`FIXME`.** | ||
| Grep density (non-experiments, non-correlate_old): the heaviest are | ||
| `nav_model_stars.py` (8), `sim_body.py` (6), `nav_master.py` (6), | ||
| `dataset_pds3.py` (6), `support/correlate.py` (4). CLAUDE.md says | ||
| TODOs are fine in principle, but many of these are ~1 year old. | ||
| Examples of still-relevant items worth triaging: | ||
| - `nav_master.py` line 78, 103, 370, 434, 486. | ||
| - `nav_model_stars.py` line 310 (unexplained `copy.deepcopy`), line | ||
| 709 (instrument-specific code path placeholder), line 741 (post- | ||
| offset value in metadata). | ||
| - `nav_technique_correlate_all.py` line 133, 335. | ||
| - `sim_body.py` lines 383, 412-415 (crater math magic numbers). | ||
|
|
||
| - **Medium — Mild deprecation risk: NumPy 2.x cast semantics.** | ||
| `pyproject.toml` requires `numpy>=2.2.0`. The code uses `np.asarray(…, | ||
| float)`, `np.array`, and masked-array operations heavily. A targeted | ||
| pass to ensure that masked-array fill values and dtype promotion | ||
| behaviours aren't shifting under NumPy 2.3+ would be worth a ticket. | ||
|
|
||
| - **Medium — Python version / platform assumptions.** | ||
| - `src/nav/config_files/config_01_general.yaml` line 13: | ||
| `truetype_font_dir: /usr/share/fonts/truetype # TODO`. Fails on | ||
| macOS (`/System/Library/Fonts/`) and Windows. The `# TODO` is | ||
| honest but long-standing. | ||
| - `subprocess.check_output(['git', 'describe', ...])` assumes `git` | ||
| on PATH. Already wrapped in try/except, low risk but platform- | ||
| dependent. | ||
|
|
||
| ## 10. Packaging and distribution | ||
|
|
||
| - **High — Version string: single source of truth is setuptools-scm, | ||
| but `src/nav/_version.py` is `write_to=...` (tracked at commit, not | ||
| generated).** | ||
| `pyproject.toml` line 194. When `setuptools_scm` is in `write_to` | ||
| mode the file should be in `.gitignore` (it isn't — `_version.py` is | ||
| missing from the current `.gitignore` listing). If a stale | ||
| `_version.py` is committed, editable installs will prefer it over | ||
| `setuptools_scm`'s dynamically computed version. | ||
|
|
||
| - **Medium — Missing classifiers.** | ||
| No `Intended Audience`, no `Topic :: Scientific/Engineering :: Image | ||
| Processing`, no `Programming Language :: Python :: 3 :: Only`. Low | ||
| priority. | ||
|
|
||
| - **Medium — `[project].description` is fine but `maintainers` has one | ||
| entry (Robert French) and no `authors` list.** | ||
| Common convention is to populate both. Not critical. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider: note the discontinuous section numbering up front.
Sections 2, 3, 5, 6, 7 are skipped. A reader unfamiliar with the parent audit outline will wonder whether content is missing. A one-line note near the top ("Numbers follow the parent audit outline; sections without findings are omitted") would eliminate the ambiguity without changing any finding text.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~24-~24: The official name of this software platform is spelled with a capital “H”.
Context: ...dist=loadfile!) and the CI workflow at .github/workflows/run-tests.yml` line 91 uses b...
(GITHUB)
[grammar] ~65-~65: Ensure spelling is correct
Context: ...st check a lower bound (>= 8868) or be parametrised with the expected volume cut. - **Medi...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[uncategorized] ~85-~85: The official name of this software platform is spelled with a capital “H”.
Context: ...n CI this is fine (EGL is installed per .github/workflows/run-tests.yml line 73). For ...
(GITHUB)
[uncategorized] ~103-~103: The official name of this software platform is spelled with a capital “H”.
Context: ...r Renovate configuration (nothing under .github/ matches); also explicitly called ...
(GITHUB)
🪛 markdownlint-cli2 (0.22.1)
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CODEBASE_ANALYSIS.md` around lines 1 - 188, The analysis doc's section
numbering jumps (sections 2,3,5,6,7 missing) which can confuse readers; edit
CODEBASE_ANALYSIS.md near the top just before "## 1. Structure and layout" to
add a single-line explanatory note such as "Numbers follow the parent audit
outline; sections without findings are omitted" so readers know the omission is
intentional; ensure the note is concise and placed immediately above or below
the document title/header so it is visible when scanning the "## 1. Structure
and layout" section.
There was a problem hiding this comment.
Actionable comments posted: 23
♻️ Duplicate comments (3)
src/reproj_cli/paths.py (1)
66-80:⚠️ Potential issue | 🟠 MajorRemote FCPath URIs (
gs://,s3://,file://) are still silently mis-resolved.This concern was raised in the previous review round and does not appear to be addressed:
Path(str(FCPath(output_dir))).expanduser().resolve()treats a URI likegs://bucket/outas a local relative path, and the returnedFCPath(str(candidate))then points to a local directory rather than the intended cloud location. Since the type signature advertisesstr | FCPathand the rest of the codebase supportsrms-filecacheURIs for output roots, callers passing--output-dir gs://...will silently write to the wrong place.Either reject URIs with a scheme up-front with a clear
ValueError, or perform cloud-aware normalization viaFCPaththroughout without dropping intopathlib.🛡️ Proposed guard
def _ensure_output_under_dir(output_dir: str | FCPath, filename: str) -> FCPath: if not filename or filename in {'.', '..'}: raise ValueError(f'invalid output filename: {filename!r}') if '/' in filename or '\\' in filename: raise ValueError(f'filename must not contain path separators: {filename!r}') - base = Path(str(FCPath(output_dir))).expanduser().resolve() + od_str = str(FCPath(output_dir)) + if '://' in od_str: + raise ValueError( + f'output_dir must be a local path; remote URIs are not supported: {od_str!r}' + ) + base = Path(od_str).expanduser().resolve() candidate = (base / filename).resolve()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/reproj_cli/paths.py` around lines 66 - 80, The function _ensure_output_under_dir currently converts FCPath to a local pathlib.Path (via Path(str(FCPath(output_dir))).expanduser().resolve()), which silently mis-resolves remote URIs like gs:// or s3://; change it to detect and reject remote-scheme URIs up-front instead of treating them as local paths: construct an FCPath from output_dir, inspect its scheme (or otherwise detect "://" in the string) and if the scheme is non-empty and not a local 'file' scheme raise a ValueError clearly explaining that remote URIs are not allowed for output_dir; keep the existing local-path logic (base, candidate, candidate.relative_to(base)) only for truly local FCPath/paths and avoid using pathlib.Path on remote URIs.src/nav/reproj/rings.py (1)
642-645:⚠️ Potential issue | 🟠 MajorUse
_MAX_LONGITUDEhere;2*pistill allocates a duplicate full-circle bin.For resolutions that divide
2*piexactly,floor(2*pi / res) + 1produces one more column thanreproject()andgenerate_longitudes()can ever populate, because both cap bins atfloor(_MAX_LONGITUDE / res).to_full()then exposes a spurious extra masked column at the 360 deg edge.Possible fix
- self._n_full_lon = math.floor(2.0 * math.pi / longitude_resolution) + 1 + self._n_full_lon = math.floor(_MAX_LONGITUDE / longitude_resolution) + 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nav/reproj/rings.py` around lines 642 - 645, The calculation of _n_full_lon currently uses 2.0 * math.pi which can over-allocate a duplicate full-circle bin; change the computation that sets self._n_full_lon to use _MAX_LONGITUDE (i.e. floor(_MAX_LONGITUDE / longitude_resolution) + 1) so it matches the capping behavior in reproject(), generate_longitudes(), and to_full() and avoids creating the spurious masked 360° column.src/main/nav_mosaic.py (1)
152-174:⚠️ Potential issue | 🟠 MajorThe broad per-image
except Exceptionturns code regressions into silent skips.Each handler covers load, offset application, reprojection/add, and save in one block and then just logs + continues. That means a real bug in the new workflow can still "succeed" with a partial mosaic instead of failing fast. Catch only the expected data/IO failures at the narrowest points and let unexpected exceptions abort.
As per coding guidelines, "ALWAYS catch exceptions at the smallest granularity possible. Do NOT wrap large blocks in a single
try/except."Also applies to: 392-406, 467-477
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/nav_mosaic.py` around lines 152 - 174, The per-image try/except is too broad and hides unexpected bugs; narrow it by catching only anticipated IO/data errors around image loading (obs_class.from_file and load_offset_if_any) and file writes (result.save) while letting unexpected exceptions propagate (remove the blanket except Exception around the whole block). Refactor so load and offset application are wrapped in a small try that logs via _log_main_exception on expected errors, wrap the save block keyed by args.no_write_output_files and MAIN_LOGGER in its own try for filesystem errors, and keep reproject_fn/processing (obs_inst, reproject_fn, n_done increment) outside those catches so real regressions surface instead of being silently skipped. Ensure you still reference image_file.image_file_url in logged IO errors to preserve context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/user_guide_reprojection.rst`:
- Around line 67-84: The docs incorrectly state that the ``time`` array defaults
to float32; update the paragraph describing defaults so it excludes ``time``
from "all geometry arrays (resolution, phase, emission, incidence) and the
``time`` array use ``float32``" and instead say that geometry arrays
(resolution, phase, emission, incidence) default to ``float32`` while the
``time`` field is always stored as ``float64`` regardless of the
``metadata_dtype`` argument to BodyMosaic; keep references to the
``image_dtype`` and ``metadata_dtype`` options intact.
In `@pyproject.toml`:
- Around line 85-88: Remove the global disable_error_code = ["import-untyped"]
setting from pyproject.toml so we stop suppressing untyped-import diagnostics
project-wide; instead leave per-module ignore_missing_imports = true entries
(e.g., for astropy/scipy/oops) intact and ensure strict = true remains in the
config, preserving targeted overrides rather than a blanket disable_error_code
override.
In `@scripts/read_docs.sh`:
- Around line 48-51: The Linux branch currently runs xdg-open "$path" without
checking availability; update the case for "Linux" to first detect if xdg-open
exists (e.g., using command -v or which) and if not present, fall through to the
catch-all/manual-open branch (or invoke the same manual message) so the script
does not fail with "xdg-open: command not found"; reference the xdg-open
invocation and the "$path" use and ensure behavior mirrors the existing
default/manual branch when xdg-open is absent.
- Line 3: Replace the literal placeholder token "REPONAME" in the script header
comment with the actual repository/project name so the comment reads correctly;
edit the top-line comment that currently contains "# REPONAME - Build Sphinx
documentation and open the HTML index" and substitute REPONAME with the real
repo name (keeping the rest of the comment intact).
In `@src/main/nav_mosaic_cloud_tasks.py`:
- Around line 96-102: The current code calls get_nav_results_root(...) and
constructs a new FileCache(None) inside the per-task hot path; hoist that work
so it runs once during worker initialization (e.g. in async_main or module
scope) and then pass the resulting FCPath into each task via worker_data or a
closure. Specifically, resolve nav_results_root_str once with
get_nav_results_root(cli_args, DEFAULT_CONFIG), create the single
FileCache(None).new_path(...) to produce nav_results_root_path, and remove those
calls from process_task so process_task uses the precomputed
nav_results_root_path supplied via worker_data/closure.
- Around line 216-217: Add docstrings to the public functions async_main and
main: for async_main describe its async CLI entry behavior (what args it reads
or expects), side effects including that it may mutate sys.argv and sets the
module-level _MODE, and describe exit behavior (whether it returns, raises, or
calls sys.exit). Do the same for main (setuptools entry point) explaining how it
wraps async_main, its CLI surface, any argument parsing it performs, side
effects (mutating sys.argv and setting _MODE) and final process exit behavior;
place the docstrings immediately under each def and keep them concise and
conformant to the module docstring style.
- Around line 121-141: The code uses getattr on the argparse Namespace
(task_args) for constant keys; instead read these values directly from the
original dict (task_arguments) using task_arguments.get('output_dir'),
.get('prefix',''), .get('format','fits'), .get('overwrite', False),
.get('no_write_output_files', False), and .get('image_name', None), only
construct the Namespace (task_args = argparse.Namespace(**task_arguments)) when
calling build_body_mosaic or build_ring_mosaic; keep the same variable names
(output_dir_str, prefix, fmt, overwrite, no_write_output_files,
image_name_override) and still convert output_dir_str into
FCPath(output_dir_str) and dispatch to build_body_mosaic/build_ring_mosaic based
on _MODE.
In `@src/nav/dataset/dataset_pds3_galileo_ssi.py`:
- Around line 71-72: The conditional that strips the volume prefix currently
checks parts[0].startswith('GO_') and is case-sensitive; change it to use a
case-insensitive check (e.g., parts[0].upper().startswith('GO_')) so lowercase
prefixes like "go_0002/…" are also stripped the same way as Cassini's
.upper().startswith('COISS_') logic—update the if that references parts[0] to
call .upper() before startswith.
In `@src/nav/reproj/_serialization.py`:
- Around line 313-319: Check for the presence of sentinel keys before indexing
to avoid KeyError: when loading with np.load (the blocks that index '__kind__'
and '__version__' and the other loader that uses 'KIND'/'VERSION'), first assert
those keys exist (e.g. via "if '__kind__' not in raw: raise ValueError('Missing
file sentinel __kind__ - file is truncated or wrong format')" and similarly for
'__version__', 'KIND', and 'VERSION'), then convert to str/int and validate the
values as currently done; update both occurrences referenced in the diff so
missing sentinels raise a clear ValueError describing schema/corruption instead
of propagating KeyError.
In `@src/nav/reproj/bodies.py`:
- Around line 109-166: Rename the shadowing parameter format to format_ in
BodyReprojResult.save and the corresponding BodyReprojResult.load, plus the same
change in BodyMosaicData.save/load and any other serialization/signature that
currently exposes format (including helpers that call infer_format or accept
that kwarg), remove the "# noqa: A002" markers, and update all internal uses and
calls (e.g., calls to infer_format(path, format) and save_npz/save_fits
invocations) to reference format_ instead so the public API remains consistent
and no built-in is shadowed; ensure any downstream callers and tests are updated
to use format_ as the keyword.
- Around line 546-624: The __init__ of BodyMosaic currently validates only
latlon_type/lon_direction/merge_strategy but must also validate numeric inputs:
check lat_resolution and lon_resolution are > 0 (raise ValueError in __init__),
ensure zoom is >= 1 and edge_margin is >= 0, and validate optional
max_incidence/max_emission are within [0, pi] (or non-negative for
max_resolution and > 0 if representing km/pixel) and of correct numeric types;
add explicit type checks (int/float) where appropriate and raise TypeError for
wrong types and ValueError for out-of-range values before computing
_n_full_lat/_n_full_lon so functions like math.floor, array_zoom, and slicing
never receive invalid values (refer to __init__, attributes _lat_resolution,
_lon_resolution, _zoom, _edge_margin, _max_incidence, _max_emission, and
_max_resolution).
- Around line 1139-1204: The code uses ok_body_mask shape to offset
good_u/good_v into full-frame coordinates but still indexes backplane arrays
(bp_incidence, bp_emission, bp_phase, resolution) with subimage-relative
good_u/good_v, causing wrong samples when backplanes are full-frame; fix by
computing full-frame indices the same way you compute ok_on_detector (i.e.,
replace bp_*[good_v, good_u] and resolution[good_v, good_u] with bp_*[good_v +
v_min, good_u + u_min] / resolution[good_v + v_min, good_u + u_min] when
ok_body_mask.shape != subimg.shape, or alternatively validate/reject the
mismatched shape in reproject()), and apply the same adjustment for all uses of
bp_incidence, bp_emission, bp_phase, and resolution before assigning to
repro_img_full / repro_res_full / repro_phase_full / repro_emission_full /
repro_incidence_full.
In `@src/nav/reproj/photometric_model.py`:
- Around line 153-156: The denominator handling is inconsistent: make both
correct() and uncorrect() compute a single floored denom for (cos_i + cos_e)
using the configured min_denom but preserve the original sign (e.g., denom =
np.where(np.abs(raw_denom) < min_denom, np.sign(raw_denom) * min_denom,
raw_denom)), keep the existing cos_i floor (min_cos_incidence) logic, and use
that signed, floored denom in the division in both correct() and uncorrect() so
near-zero negative sums do not flip sign or produce huge values; update the
calculations that reference cos_i, cos_e, min_cos_incidence, and min_denom
accordingly.
In `@src/nav/reproj/ring_orbit_model.py`:
- Around line 65-69: The code in ring_orbit_model currently swallows a TypeError
from _utc2et and silently sets _epoch_et to 0.0; instead validate epoch_utc and
propagate a clear error: remove the broad try/except that sets epoch_et=0.0,
call _utc2et(self.epoch_utc) and if it raises TypeError or returns an invalid
value raise a new TypeError or ValueError with a descriptive message referencing
epoch_utc; ensure object.__setattr__(self, '_epoch_et', ...) only runs with a
valid epoch_et so the RingOrbitModel (or class containing epoch_utc/_epoch_et)
cannot be constructed with a coerced J2000 epoch.
In `@src/nav/reproj/rings.py`:
- Around line 929-944: Validate longitude_range and radius_range before using
them: when deriving longitude_start/longitude_end and radius_inner/radius_outer
(the variables set in this block), first check types and finiteness (no
NaN/inf), enforce 0 <= longitude_start <= longitude_end <= _MAX_LONGITUDE, and
enforce radius_inner < radius_outer (taking into account whether orbit_model is
None or offsets apply); on invalid inputs raise clear ValueError or TypeError
with descriptive messages. Also ensure radius_range values are floats (cast only
after validation) and preserve fallback to self._radius_inner/self._radius_outer
when radius_range is None.
- Around line 1368-1397: The add() validation misses verifying that the number
of true entries in repro.longitude_antimask equals the number of sparse columns
in repro.img; add a check in RingMosaic.add that
int(np.count_nonzero(repro.longitude_antimask)) == int(repro.img.shape[1]) and
raise a ValueError with a clear message if not (mention both counts), so
corrupted/hand-constructed RingReprojResult objects are rejected before reaching
_insert_new_columns().
In `@src/nav/ui/mosaic_viewer/body_window.py`:
- Around line 286-289: The statusBar method lacks a Google-style docstring
explaining its behavior and return contract; add a docstring to the
statusBar(self) -> QStatusBar method that describes it overrides the Qt method,
notes that it calls super().statusBar(), asserts non-None (so callers can rely
on a QStatusBar rather than Optional), and includes a Returns: section
specifying it returns a QStatusBar and that the assert narrows the type from
QStatusBar | None to QStatusBar.
In `@src/nav/ui/mosaic_viewer/graticule.py`:
- Around line 71-86: The while loops that increment lat and lon (using lat +=
lat_step_deg and lon += lon_step_deg) can accumulate floating-point error;
replace them with integer-driven loops computed from a step count (e.g.,
n_lat_steps = int(round(180.0 / lat_step_deg)) and then for i in
range(n_lat_steps+1): lat = -90.0 + i * lat_step_deg) and similarly for
longitudes (compute n_lon_steps from 360.0 / lon_step_deg and iterate i to
compute lon = i * lon_step_deg), then call lonlat_to_display and _split_polyline
as before; apply the same change to the analogous loops in
graticule_label_anchors to use deterministic integer indices instead of floating
increments.
In `@src/nav/ui/mosaic_viewer/ring_window.py`:
- Around line 947-954: The comment describing the image widget behavior around
ring_full_lon=True contains a literal degree sign (°); replace that unicode
character with the ASCII text "deg" so the phrase reads "longitude 0 deg".
Update the comment near the ring_full_lon=True / virtual column 0 explanation
(the block referencing "virtual column 0 is at longitude 0°") to use "longitude
0 deg" instead, keeping the rest of the wording unchanged.
- Around line 104-115: The function _compute_ewmu currently calls
emission_deg.filled(0.0) which replaces masked emission angles with 0° and
produces mu=1.0, losing the emission mask and producing bogus unmasked ew*mu
values; change the computation to be mask-aware by computing mu as a masked
array that carries emission_deg.mask (e.g., use masked-array-aware operations or
construct mu with ma.array(..., mask=emission_deg.mask)), then combine masks so
the returned ew*mu also inherits emission_deg’s mask (and preserve ew’s mask as
well) so full_ewmu and tooltips don't show unmasked bogus values.
In `@src/nav/ui/mosaic_viewer/sphere_render.py`:
- Around line 159-163: The expression building the no_data mask (no_data = valid
& (~inside | tile_mask)) is precedence-sensitive; ensure the parentheses around
(~inside | tile_mask) are preserved and add a brief inline comment next to the
no_data assignment explaining that & must bind to the whole parenthesized
expression (i.e., we want "valid AND (not inside OR tile_mask)"), so future
refactors don't remove the parentheses and change semantics for valid, inside,
and tile_mask.
In `@src/nav/ui/mosaic_viewer/tiled_image_widget.py`:
- Around line 1186-1190: The color-tint indexing is inverted when self._y_flip
is True because the extra [::-1] reverses the already-descending indices; change
the y-flip branch so arr_rows is set to the clipped reversed indices directly
(use np.clip((n_r - 1) - rows, 0, n_r - 1)) instead of applying a trailing
[::-1], ensuring arr_rows aligns with tile = tile_raw[::-1, :] and that tint =
self._color_tint[np.ix_(arr_rows, cols)] picks matching rows for non-symmetric
color overlays.
In `@src/reproj_cli/offsets.py`:
- Around line 71-77: The current broad except Exception around the read_text
call swallows non-IO bugs; narrow the handler to only the realistic exceptions
from filecache.FCPath.read_text() (e.g., OSError and UnicodeDecodeError) so
programming errors still propagate. Locate the try/except that logs via
MAIN_LOGGER.warning with image_file.image_file_url and replace the bare except
Exception with specific except blocks (or a tuple except) for OSError and
UnicodeDecodeError, keeping the same warning message and return None behavior.
---
Duplicate comments:
In `@src/main/nav_mosaic.py`:
- Around line 152-174: The per-image try/except is too broad and hides
unexpected bugs; narrow it by catching only anticipated IO/data errors around
image loading (obs_class.from_file and load_offset_if_any) and file writes
(result.save) while letting unexpected exceptions propagate (remove the blanket
except Exception around the whole block). Refactor so load and offset
application are wrapped in a small try that logs via _log_main_exception on
expected errors, wrap the save block keyed by args.no_write_output_files and
MAIN_LOGGER in its own try for filesystem errors, and keep
reproject_fn/processing (obs_inst, reproject_fn, n_done increment) outside those
catches so real regressions surface instead of being silently skipped. Ensure
you still reference image_file.image_file_url in logged IO errors to preserve
context.
In `@src/nav/reproj/rings.py`:
- Around line 642-645: The calculation of _n_full_lon currently uses 2.0 *
math.pi which can over-allocate a duplicate full-circle bin; change the
computation that sets self._n_full_lon to use _MAX_LONGITUDE (i.e.
floor(_MAX_LONGITUDE / longitude_resolution) + 1) so it matches the capping
behavior in reproject(), generate_longitudes(), and to_full() and avoids
creating the spurious masked 360° column.
In `@src/reproj_cli/paths.py`:
- Around line 66-80: The function _ensure_output_under_dir currently converts
FCPath to a local pathlib.Path (via
Path(str(FCPath(output_dir))).expanduser().resolve()), which silently
mis-resolves remote URIs like gs:// or s3://; change it to detect and reject
remote-scheme URIs up-front instead of treating them as local paths: construct
an FCPath from output_dir, inspect its scheme (or otherwise detect "://" in the
string) and if the scheme is non-empty and not a local 'file' scheme raise a
ValueError clearly explaining that remote URIs are not allowed for output_dir;
keep the existing local-path logic (base, candidate,
candidate.relative_to(base)) only for truly local FCPath/paths and avoid using
pathlib.Path on remote URIs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0b5e3710-84ea-4d1b-921e-a0c0936d98c4
📒 Files selected for processing (40)
README.mddocs/api_reference/api_reproj.rstdocs/developer_guide_reprojection.rstdocs/introduction_overview.rstdocs/user_guide_backplanes.rstdocs/user_guide_navigation.rstdocs/user_guide_reprojection.rstpyproject.tomlscripts/read_docs.shsrc/backplanes/backplanes.pysrc/backplanes/writer.pysrc/main/nav_backplanes.pysrc/main/nav_mosaic.pysrc/main/nav_mosaic_cloud_tasks.pysrc/main/nav_mosaic_display.pysrc/nav/config_files/config_31_inst_gossi.yamlsrc/nav/dataset/dataset_pds3.pysrc/nav/dataset/dataset_pds3_galileo_ssi.pysrc/nav/reproj/_serialization.pysrc/nav/reproj/bodies.pysrc/nav/reproj/cartographic_model.pysrc/nav/reproj/photometric_model.pysrc/nav/reproj/ring_orbit_model.pysrc/nav/reproj/rings.pysrc/nav/ui/__init__.pysrc/nav/ui/common.pysrc/nav/ui/mosaic_viewer/__init__.pysrc/nav/ui/mosaic_viewer/body_window.pysrc/nav/ui/mosaic_viewer/common.pysrc/nav/ui/mosaic_viewer/graticule.pysrc/nav/ui/mosaic_viewer/matplotlib_qt.pysrc/nav/ui/mosaic_viewer/ring_window.pysrc/nav/ui/mosaic_viewer/sphere_render.pysrc/nav/ui/mosaic_viewer/tiled_image_widget.pysrc/reproj_cli/offsets.pysrc/reproj_cli/paths.pytests/nav/reproj/test_bodies.pytests/nav/reproj/test_rings.pytests/nav/reproj/test_shared_modules.pytests/nav/ui/test_common.py
There was a problem hiding this comment.
Actionable comments posted: 20
♻️ Duplicate comments (2)
scripts/read_docs.sh (1)
48-51:⚠️ Potential issue | 🟡 MinorHandle Linux hosts without
xdg-open.This still assumes a desktop opener is installed. On headless servers or minimal containers, the script will fail with
xdg-open: command not foundinstead of falling back to the manual-open message you already have in the default branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/read_docs.sh` around lines 48 - 51, In the Linux branch of the case statement (the "Linux)" block) check for xdg-open before calling it: use a command existence test like `command -v xdg-open >/dev/null 2>&1` (or `which`/`type`) and if present run `xdg-open "$path"`, otherwise print the existing manual-open fallback message; update the Linux) block around the use of "$path" to perform this conditional so headless/minimal environments won't error with "xdg-open: command not found".src/main/nav_mosaic.py (1)
130-130: 🛠️ Refactor suggestion | 🟠 MajorStop relying on module-global dataset state here.
parse_args()still mutatesDATASET/DATASET_NAME, and the rest of the workflow reads them back indirectly. That keepsmain()non-reentrant, leaks state across repeated in-process invocations, and is the reason for theassert/# type: ignore[arg-type]scaffolding in the body/rings runners. Return the dataset instance and dataset name fromparse_args()and thread them through_write_cloud_tasks_file(),_run_reproject_pass(),_run_body(), and_run_rings()instead.Based on learnings: "Avoid mutable global variables. If unavoidable, document purpose and limit scope. Prefer module-level constants (ALL_CAPS) or dependency injection."
Also applies to: 185-186, 267-275, 314-343, 351-355, 441-445
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/nav_mosaic.py` at line 130, parse_args() currently mutates module globals DATASET and DATASET_NAME which makes main() non-reentrant; change parse_args() to return (dataset, dataset_name) instead of setting globals, then update main() to accept those returns and pass them as parameters into _write_cloud_tasks_file(), _run_reproject_pass(), _run_body(), and _run_rings(); adjust the signatures of those helper functions to accept a dataset and dataset_name (or a single dataset object that exposes its name) and remove usages of the module-level DATASET/DATASET_NAME within them and their callers so no code path reads the global DATASET; ensure all call sites (including any runners noted around lines 185-186, 267-275, 314-343, 351-355, 441-445) are updated to thread the returned values through.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/developer_guide_reprojection.rst`:
- Around line 127-144: The docs currently say ``_metadata_dtype`` applies to all
geometry arrays and "and for ``time``", but later states ``time`` is always
``float64``; update the paragraph to remove or correct the claim that
``_metadata_dtype`` applies to ``time`` so the contract is consistent: state
that ``_metadata_dtype`` (default ``np.float32``) applies to geometry arrays
(``resolution``, ``eff_resolution``, ``phase``, ``emission``, ``incidence``) but
that ``time`` is always stored as ``float64`` (this should be reflected
consistently in the descriptions around ``reproject()``, the cast of
``bp_xyz.mvals.astype(self._metadata_dtype)``, and the fields on
``BodyReprojResult`` / ``BodyMosaicData``).
In `@docs/user_guide_reprojection.rst`:
- Around line 78-83: The comment at the top of the BodyMosaic example is
inconsistent with the actual dtypes: update the comment to match the example
values (or change the example to match the comment). Specifically, in the
BodyMosaic instantiation referencing image_dtype and metadata_dtype, either
change the header comment from "All float64 for maximum precision" to reflect
that image_dtype is np.float32 and metadata_dtype is np.float64 (e.g., "image
stored as float32, metadata as float64") or change image_dtype to np.float64 so
both dtypes are float64; adjust the surrounding text accordingly to keep the
example and comment consistent.
In `@src/main/nav_mosaic_cloud_tasks.py`:
- Around line 122-141: Check and validate types of the incoming task_data
payload before accessing nested fields: ensure task_data is a dict, that
task_data.get('files') is a list, that task_data.get('arguments') is a dict, and
that each element in files is a dict; if any check fails, return the structured
error tuple (False, {'status':'error', 'status_error': '<specific_error>'}) or
raise a clear TypeError/ValueError as appropriate. Update the code paths around
variables/files/ task_arguments/output_dir_str (and the later file-entry
handling at the block around lines 148-152) to perform these type checks prior
to calling .get(), and only call FCPath(output_dir_str) and
argparse.Namespace(**task_arguments) after validating output_dir_str is a string
and task_arguments is a dict. Ensure error messages use precise identifiers like
'no_files', 'invalid_files_type', 'no_arguments', 'invalid_arguments_type',
'no_output_dir', or 'invalid_output_dir_type' to keep responses structured.
- Around line 174-179: The construction of image_log_path uses
image_file.results_path_stub directly which allows path traversal; sanitize and
validate the stub before building the filename: derive a safe filename from
image_file.results_path_stub (e.g., take os.path.basename or replace path
separators and disallowed chars) and then construct image_log_path =
FCPath(output_dir) / 'logs' / (safe_stub + '_' + timestamp + '.log'); after
creation, resolve the resulting path to an absolute path and verify it is a
child of (FCPath(output_dir) / 'logs').resolve() (raise an exception or fall
back to a safe default stub if not) before calling
image_log_path.parent.mkdir(...) and passing image_log_path to
image_log_handlers so traversal cannot escape output_dir/logs.
- Around line 213-218: The per-image reprojection exceptions are being swallowed
and the function still reports success; add failure aggregation by introducing a
flag or list (e.g., had_reprojection_failure or reprojection_errors) inside the
reprojection loop, set it in the except block where _log_main_exception is
called for image_file, and after the loop use that flag to return an overall
failure (do not return success when any per-image reprojection failed). Also
include relevant error details in the aggregated result or choose the
appropriate retry/None return value so callers can detect the overall failure
(refer to image_file, _log_main_exception, MAIN_LOGGER, image_log_path, and the
existing final return).
In `@src/nav/reproj/_serialization.py`:
- Around line 186-205: orbit_model_from_dict currently indexes required keys
directly causing KeyError on malformed input; update orbit_model_from_dict to
first validate that either d.get('is_none') is True or that all required keys
('name','a','e','w0','dw','mean_motion','epoch_utc') are present, and if any are
missing raise a ValueError with a clear message listing missing fields; keep
existing casts and construction of RingOrbitModel and reference
orbit_model_to_dict in the error message/context if helpful.
In `@src/nav/reproj/bodies.py`:
- Around line 943-954: In reproject (method reproject) validate the
navigation_uncertainty parameter at the API boundary: check that
navigation_uncertainty is a finite number and >= 0 before it is used (e.g.,
before computing eff_resolution); if the value is not a number raise TypeError,
and if it is finite but < 0 raise ValueError with a clear message stating the
requirement (finite and >= 0), so downstream logic that computes 1.0 +
navigation_uncertainty cannot produce negative or non-finite effective
resolutions.
- Around line 1192-1219: The mask_only branch builds repro_img at the full
latitude×longitude shape but returns lat_idx_range/lon_idx_range and metadata
arrays for the cropped window, causing shape mismatch; modify this branch in the
mask_only handling (around repro_img, good_lat/good_lon and the BodyReprojResult
return) to slice/crop repro_img to the same window (min_lat_pixel:max_lat_pixel,
min_lon_pixel:max_lon_pixel) before wrapping it as
ma.MaskedArray.astype(self._image_dtype) so the returned img shape and all
metadata (lat_idx_range, lon_idx_range, resolution, eff_resolution, phase,
emission, incidence, time, etc.) are consistent with the cropped indices.
In `@src/nav/reproj/photometric_model.py`:
- Around line 52-68: Add input validation to the photometric model dataclasses
(starting with LambertModel) by implementing a __post_init__ that checks numeric
parameters are finite and within valid ranges (e.g., min_cos_incidence is a
finite float > 0 and >= tiny threshold), using math.isfinite and type checks,
and raise ValueError/TypeError with a clear message on failure; apply the same
pattern to the other photometric model classes in this module that accept
thresholds/exponents so their parameters (thresholds, exponents, clamps) are
validated for positivity/finity.
In `@src/nav/reproj/ring_orbit_model.py`:
- Around line 58-80: The constructor currently only validates a and e but allows
NaN/inf for numeric fields; add finite checks for each numeric attribute
(self.a, self.w0, self.dw, self.mean_motion) after type checks and before
setting _epoch_et, using math.isfinite and raising ValueError with clear
messages (e.g., "RingOrbitModel <field> must be finite, got <value>") for any
non-finite value; keep existing _utc2et conversion and _epoch_et assignment
logic intact and reference the same RingOrbitModel constructor validation block
when inserting these checks.
In `@src/nav/reproj/rings.py`:
- Around line 1035-1050: The code in reproject() must validate zoom_amt before
indexing: accept either an int (or non-bool int-like) which is converted to a
2-tuple (zoom_amt, zoom_amt) or a list/tuple of exactly two items; otherwise
raise a clear TypeError/ValueError. Implement checks at the start of the block
that verify zoom_amt is int (and not bool) or a sequence of length 2, and verify
each element is an int (not bool); raise ValueError for wrong length and
TypeError for wrong element types; only after these checks set r_zoom_amt /
l_zoom_amt and r_spline_order / l_spline_order as existing code does.
- Around line 728-731: The longitude bin count calculation uses 2π directly and
adds one, producing an extra unused column; update the sizing of
self._n_full_lon to compute floor(_MAX_LONGITUDE / longitude_resolution) + 1
instead of floor(2.0 * math.pi / longitude_resolution) + 1 so the allocation
matches other bin index computations; change the expression that sets
self._n_full_lon (search for _n_full_lon, longitude_resolution and
_MAX_LONGITUDE) accordingly.
- Around line 1750-1770: to_bounded currently computes start_bin/end_bin/n_bins
directly from longitude_range and can produce negative or out-of-range
dimensions; call the existing validation helper
_validate_reproject_longitude_range(longitude_range) at the start of to_bounded
(same validation used by reproject) and use its validated output (or let it
raise ValueError) before computing start_bin, end_bin and n_bins so inputs like
reversed ranges or values outside the mosaic bounds are rejected
deterministically.
In `@src/nav/ui/mosaic_viewer/body_window.py`:
- Around line 1144-1148: The code is sampling dd.image_ma but the display may be
showing _body_view_ma after _on_photometry_changed(); update the sampling to use
self._body_view_ma (or the instance attribute that holds the currently displayed
masked array) instead of dd.image_ma, keeping the existing 'inside' check and
masked handling (ma.is_masked) and preserving the current formatting logic that
sets value_str to either the masked label or f'{float(raw_val):11.8f}'; apply
this change in the function that contains the lines referencing raw_val and
value_str so the "Value:" field always reflects the currently shown image.
In `@src/nav/ui/mosaic_viewer/common.py`:
- Around line 112-124: The .npz branch currently accesses raw['__kind__']
directly which raises KeyError for malformed files; update the code in the
function that calls infer_format and uses FCPath to load .npz (the block using
np.load and raw['__kind__']) to catch KeyError (or check for '__kind__' in raw)
and re-raise a ValueError with the same message used below ("No KIND header
found in {path!r}; expected a nav.reproj FITS export."), ensuring the .npz path
mirrors the FITS branch's behavior and returns a consistent ValueError when the
kind marker is missing.
In `@src/nav/ui/mosaic_viewer/ring_window.py`:
- Around line 1084-1088: _column_band_ewmu currently fills masked emissions and
produces unmasked mu; instead compute mu as a masked array so the emission mask
is preserved: use the masked emission dd.mean_emission (em) with mask-aware
numpy.ma trig ops (e.g. mu = np.abs(ma.cos(ma.radians(em)))) and then multiply
ma.asarray(ew) * mu so the result inherits dd.mean_emission.mask (alternatively
reuse _compute_ewmu to get a masked EW*mu and combine with _column_band_ew if
that fits the logic). Ensure you reference _column_band_ewmu, _column_band_ew,
dd.mean_emission.mask (or _compute_ewmu) when making the change.
In `@src/nav/ui/mosaic_viewer/tiled_image_widget.py`:
- Around line 177-215: _bady_sphere_lon_bin_to_dc_map currently computes
n_full_lon with int(2π / lon_res_rad) which differs from BodyMosaic's grid;
change the computation to match BodyMosaic (use floor(2π / lon_res_rad) + 1 as
used in src/nav/reproj/bodies.py / BodyMosaic.to_bounded) so the full-longitude
bin count equals the body full grid, then ensure the rest of the function
(lon_bins construction, modulo mapping into bin_to_dc, and bin_to_dc
initialization) uses that n_full_lon to avoid aliasing of the final bin onto
column 0 near the wrap seam.
In `@src/reproj_cli/offsets.py`:
- Line 61: The metadata_path construction directly concatenates
image_file.results_path_stub into FCPath(nav_results_root) allowing path
traversal; change this to resolve the stub against nav_results_root (e.g., build
a candidate path by joining then calling resolve/absolute) and then verify the
resolved path is still under the resolved nav_results_root (use is_relative_to
or compare path.parents) before calling read_text(); if the resolved metadata
path escapes nav_results_root, raise an error or reject the request rather than
reading the file.
In `@src/reproj_cli/paths.py`:
- Around line 51-62: Add explicit type checks at the start of
_validate_output_prefix and _validate_output_fmt: verify prefix and fmt are
instances of str and raise TypeError with a clear message if not, before
performing the existing validations (null-byte check, _PREFIX_INVALID.search,
and .lower()/membership check). In _validate_output_fmt call .lower() only after
the type check and keep returning the normalized ext string; in
_validate_output_prefix perform the '\x00' and path-separator checks only after
confirming prefix is a str.
- Around line 82-89: The function per_image_output_path currently exposes four
positional params; change its signature so only three are positional and the
rest are keyword-only — e.g. keep output_dir, prefix, image_file as positionals
and move fmt and subject_name after a star: def
per_image_output_path(output_dir: str | FCPath, prefix: str, image_file:
ImageFile, *, fmt: str, subject_name: str) -> FCPath; update all call sites to
pass fmt and subject_name by keyword and adjust any tests or usages accordingly.
---
Duplicate comments:
In `@scripts/read_docs.sh`:
- Around line 48-51: In the Linux branch of the case statement (the "Linux)"
block) check for xdg-open before calling it: use a command existence test like
`command -v xdg-open >/dev/null 2>&1` (or `which`/`type`) and if present run
`xdg-open "$path"`, otherwise print the existing manual-open fallback message;
update the Linux) block around the use of "$path" to perform this conditional so
headless/minimal environments won't error with "xdg-open: command not found".
In `@src/main/nav_mosaic.py`:
- Line 130: parse_args() currently mutates module globals DATASET and
DATASET_NAME which makes main() non-reentrant; change parse_args() to return
(dataset, dataset_name) instead of setting globals, then update main() to accept
those returns and pass them as parameters into _write_cloud_tasks_file(),
_run_reproject_pass(), _run_body(), and _run_rings(); adjust the signatures of
those helper functions to accept a dataset and dataset_name (or a single dataset
object that exposes its name) and remove usages of the module-level
DATASET/DATASET_NAME within them and their callers so no code path reads the
global DATASET; ensure all call sites (including any runners noted around lines
185-186, 267-275, 314-343, 351-355, 441-445) are updated to thread the returned
values through.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f5e970e1-ae1a-4982-9723-b8a73d18e473
📒 Files selected for processing (25)
docs/conf.pydocs/developer_guide_reprojection.rstdocs/user_guide_reprojection.rstpyproject.tomlscripts/read_docs.shsrc/main/nav_mosaic.pysrc/main/nav_mosaic_cloud_tasks.pysrc/main/nav_mosaic_display.pysrc/nav/dataset/dataset_pds3_galileo_ssi.pysrc/nav/reproj/__init__.pysrc/nav/reproj/_serialization.pysrc/nav/reproj/bodies.pysrc/nav/reproj/photometric_model.pysrc/nav/reproj/ring_orbit_model.pysrc/nav/reproj/rings.pysrc/nav/ui/mosaic_viewer/body_window.pysrc/nav/ui/mosaic_viewer/common.pysrc/nav/ui/mosaic_viewer/graticule.pysrc/nav/ui/mosaic_viewer/ring_window.pysrc/nav/ui/mosaic_viewer/tiled_image_widget.pysrc/reproj_cli/offsets.pysrc/reproj_cli/paths.pytests/nav/reproj/test_bodies.pytests/nav/reproj/test_rings.pytests/nav/reproj/test_serialization_fits.py
💤 Files with no reviewable changes (1)
- docs/conf.py
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (1)
src/main/nav_mosaic_cloud_tasks.py (1)
236-273:⚠️ Potential issue | 🟠 MajorDo not return task success when one or more reprojections failed.
The per-image
exceptblock only logs, and the function still returns{'status': 'success'}unconditionally. That makes a task with dropped outputs indistinguishable from a fully successful one, so the queue runner cannot alert or retry on partial failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/nav_mosaic_cloud_tasks.py` around lines 236 - 273, The per-image except block currently only logs via _log_main_exception and the function always returns {'status': 'success'}; add a boolean flag (e.g., had_error = False) before the loop and set had_error = True inside the except for each failed image (include image_file.image_file_url in the log if helpful), then after the processing loop change the final return to inspect had_error and return a non-success status (e.g., {'status': 'failed'} or {'status': 'partial_failure'}) when any reprojection failed while keeping the existing retry semantics (the outer return signature should remain (bool, dict)). Ensure you update any tests or callers that expect the status string accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/developer_guide_reprojection.rst`:
- Around line 334-342: The docs mention that nav_mosaic_cloud_tasks.py registers
add_ring_args/add_body_args based on _MODE, but the current
nav_mosaic_cloud_tasks.py parser only exposes --config-file and
--nav-results-root; update the text to state that the cloud-task worker uses a
minimal parser (only --config-file and --nav-results-root) and does not register
add_ring_args/add_body_args via _MODE, and clarify that the follow-up invocation
must be "nav_mosaic <mode> <dataset_name> --skip-reproject" (include the
required <dataset_name> argument) to match the current nav_mosaic.py CLI
surface; reference nav_mosaic_cloud_tasks.py, _MODE,
add_ring_args/add_body_args, and nav_mosaic.py in the updated note.
In `@src/main/nav_mosaic_cloud_tasks.py`:
- Around line 142-153: Validate that dataset_name, image_file_url,
label_file_url, and results_path_stub are strings before calling
dataset_name_to_inst_name or constructing FCPath; e.g., add isinstance checks
for dataset_name (before dataset_name_to_inst_name) and for
image_file_url/label_file_url/results_path_stub (before any FCPath or path
construction) and if a value is not a str return False with a clear status_error
such as 'invalid_dataset_name', 'invalid_image_file_url',
'invalid_label_file_url', or 'invalid_results_path_stub'. Keep the existing
KeyError handling around dataset_name_to_inst_name, and apply the same
string-type validation to the referenced block around lines 187-208 so malformed
non-string inputs are rejected early.
In `@src/main/nav_mosaic.py`:
- Around line 84-89: The image log path construction uses
image_file.results_path_stub directly (in the image_log_path/FCPath block) which
allows path traversal; sanitize the stub (e.g., strip path separators, use
os.path.basename or a whitelist/slug function) and then build the log path from
the sanitized name, resolve the final path to an absolute path, and verify that
the resolved path is inside the intended logs directory
(resolve(FCPath(output_dir)/'logs') and ensure
image_log_path.resolve().is_relative_to(logs_dir.resolve()) or equivalent)
before calling image_log_path.parent.mkdir() or passing it to
image_log_handlers; keep references to image_file.results_path_stub, FCPath,
image_log_path, output_dir, and image_log_handlers when making the changes.
In `@src/nav/reproj/_serialization.py`:
- Around line 355-380: Detect and fail fast on unmatched mask/data sentinels: in
the load_npz() logic where data_keys, mask_keys, none_keys, ma_keys and handled
are computed, validate that data_keys == mask_keys and if not raise a ValueError
listing the orphan base names (the symmetric difference of data_keys and
mask_keys) so files that contain "<name>__data" without "<name>__mask" or vice
versa are rejected immediately; likewise, in load_fits() add a similar check
that for every HDU name ending with "_MASK" there exists the corresponding base
HDU name (and raise a ValueError naming the missing bases) to prevent accepting
orphaned "<NAME>_MASK" entries. Ensure the error messages reference the sentinel
suffixes ("__data"/"__mask" and "_MASK") and use the existing variables/sets
(data_keys, mask_keys, ma_keys, handled) to build the validation and message.
- Around line 270-272: The np.savez/_compressed call can implicitly append
“.npz” when given a string path, causing a mismatch with fcpath.upload() which
uses the original local_path; to fix, open local_path in binary write mode and
pass that file handle to save_fn (save_fn(filehandle, **arrays)) instead of the
path string, ensuring you call save_fn with the same open file object for both
compress and non-compress branches and then call fcpath.upload() on local_path
so the written file and the uploaded path are identical; update the code around
save_fn, local_path, arrays, compress, and fcpath.upload to use this file-handle
approach.
In `@src/nav/reproj/rings.py`:
- Around line 718-721: The docstrings undercount the mosaic cap by one: update
the wording in both places that reference the max contributing images for
image_number and add() to state 65 536 (i.e., image_number spans 0..65535 for a
total of 65,536 images). Mention image_number, _image_count and add() in the
docstring edits so readers understand that _image_count is stored before
incrementing and that the valid image_number range is 0..65535 (total 65,536),
causing an OverflowError only after the 65,536th image is added.
- Around line 1611-1614: The running mean gets permanently corrupted if
repro.incidence is non-finite; update add() and validation to handle this by
either rejecting non-finite incidences in _validate_repro_compatible or by
skipping them when accumulating the running mean: add an integer counter (e.g.
self._mean_incidence_count) initialized alongside self._mean_incidence_sum in
__init__, only increment the count and add to self._mean_incidence_sum when
math.isfinite(repro.incidence) (or numpy.isfinite) in add(), compute
self._mean_incidence as
self._mean_incidence_sum/float(self._mean_incidence_count) when the count>0
(otherwise set to 0 or None), and ensure any persistence/readback logic that
relies on mean_incidence uses the same sum+count semantics so NaN incidences do
not taint the stored mean.
In `@src/nav/ui/mosaic_viewer/body_window.py`:
- Around line 657-677: The nested subclass _ZoomSync inside _make_zoom_sync
causes a new class per call and lacks docstrings; hoist a single _ZoomSync class
to module scope (adjacent to other top-level helpers) that subclasses
_SyncedSlider and implements _to_slider and _from_slider using _zoom_to_slider
and _slider_to_zoom, and add class and method docstrings; update _make_zoom_sync
(and any uses in BodyMosaicWindow) to instantiate the module-level _ZoomSync
instead of defining it inline.
In `@src/nav/ui/mosaic_viewer/common.py`:
- Around line 50-92: The helper _ring_longitude_column_origin_and_extent_hi_deg
is collapsing sparse longitude bins with gaps into a contiguous origin/extent
pair which compresses holes; instead, detect when longitude_antimask has holes
(global_bins = np.flatnonzero(longitude_antimask) and global_bins.size == n_cols
but not contiguous) and stop returning a single origin+extent for that
case—return a clear signal (e.g., raise or return None/sentinel) so callers
(such as RingDisplayData construction) store and use the actual populated bin
indices (global_bins) rather than computing longitudes as origin + ix *
resolution; update RingDisplayData creation paths to accept and preserve
global_bins (np.flatnonzero(longitude_antimask)) for sparse/discontiguous
mosaics and only use origin/extent when bins are contiguous or longitude_range
is provided.
In `@src/nav/ui/mosaic_viewer/ring_window.py`:
- Around line 1354-1369: In _fit_zoom_to_window, guard against dd.n_longitude ==
0 (and similarly dd.n_radii == 0) before computing x_zoom/y_zoom to avoid
division-by-zero and downstream code that assumes at least one column; if
dd.n_longitude == 0 return early after setting self._pending_fit appropriately
and put the viewer into an explicit empty-data state (e.g. clear the image via
self._image_widget.clear()/set_empty_state or set a safe default zoom and avoid
calling self._image_widget.scroll_to_pixel), so no further zoom/scroll or
cursor/EW logic runs for zero-column sparse files.
In `@src/nav/ui/mosaic_viewer/tiled_image_widget.py`:
- Around line 471-473: The computation for _ring_full_lon_cols under
ring_full_lon uses int(360.0 / x_interval), which truncates and can
under-allocate the virtual 360° canvas; change this to use a ceiling-based
calculation (e.g., math.ceil(360.0 / x_interval) cast to int) so the number of
columns truly spans 360° (keep the max(self._n_cols, ...) logic); ensure math is
imported if not already and preserve the conditional guard (ring_full_lon and
x_interval > 0) when updating the assignment in tiled_image_widget.py.
In `@src/reproj_cli/offsets.py`:
- Around line 63-75: The current offset parsing silently accepts booleans and
non-finite floats; update the logic in the offsets parsing block that handles
the local variable offset (the sequence check and the conversion of dv_raw,
du_raw) to explicitly reject bool elements and non-finite numbers: after
extracting dv_raw and du_raw, raise a TypeError if either element is a bool,
convert to float, then use math.isfinite() to validate both and raise ValueError
if either is non-finite (NaN/Infinity); ensure the function returns the
(float(dv), float(du)) only after these checks so callers see clear exceptions
instead of silently accepting bad values.
---
Duplicate comments:
In `@src/main/nav_mosaic_cloud_tasks.py`:
- Around line 236-273: The per-image except block currently only logs via
_log_main_exception and the function always returns {'status': 'success'}; add a
boolean flag (e.g., had_error = False) before the loop and set had_error = True
inside the except for each failed image (include image_file.image_file_url in
the log if helpful), then after the processing loop change the final return to
inspect had_error and return a non-success status (e.g., {'status': 'failed'} or
{'status': 'partial_failure'}) when any reprojection failed while keeping the
existing retry semantics (the outer return signature should remain (bool,
dict)). Ensure you update any tests or callers that expect the status string
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 41198aaf-bfa2-4063-b6dd-d8d9d1e3b776
📒 Files selected for processing (18)
docs/developer_guide_reprojection.rstdocs/user_guide_reprojection.rstsrc/main/nav_mosaic.pysrc/main/nav_mosaic_cloud_tasks.pysrc/nav/reproj/_serialization.pysrc/nav/reproj/bodies.pysrc/nav/reproj/photometric_model.pysrc/nav/reproj/ring_orbit_model.pysrc/nav/reproj/rings.pysrc/nav/ui/mosaic_viewer/body_window.pysrc/nav/ui/mosaic_viewer/common.pysrc/nav/ui/mosaic_viewer/ring_window.pysrc/nav/ui/mosaic_viewer/tiled_image_widget.pysrc/reproj_cli/offsets.pysrc/reproj_cli/paths.pytests/nav/reproj/test_rings.pytests/nav/ui/test_sphere_render.pytests/reproj_cli/test_paths.py
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyproject.toml (1)
11-27:⚠️ Potential issue | 🟠 MajorAdd version bounds to unversioned dependencies.
Runtime dependencies missing version bounds:
matplotlib,PyQt6,rms-imgdisp,rms-psfmodel,ruamel.yaml,scipy.Dev dependencies missing version bounds:
coverage,mypy,pytest,pytest-cov,pytest-profiling,pytest-xdist,pyinstrument,ruff.Per coding guidelines, all direct dependencies and dev/docs dependencies must specify minimum compatible versions (e.g.,
>=X.Y.Z). This ensures reproducible builds for contributors and CI environments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` around lines 11 - 27, Update pyproject.toml to add minimum version bounds for all unversioned packages: for runtime dependencies add ">=X.Y.Z" style lower bounds to matplotlib, PyQt6, rms-imgdisp, rms-psfmodel, ruamel.yaml, and scipy in the dependencies list, and for dev dependencies add minimum versions for coverage, mypy, pytest, pytest-cov, pytest-profiling, pytest-xdist, pyinstrument, and ruff; use the project’s supported minimum compatible versions (or known safe minima) and ensure the format matches the existing entries (e.g., "package>=X.Y.Z") so all direct runtime and dev/docs dependencies have explicit lower bounds.
♻️ Duplicate comments (4)
src/main/nav_mosaic_cloud_tasks.py (1)
254-291:⚠️ Potential issue | 🟠 MajorReturn task failure when any reprojection fails.
Per-image exceptions are logged and swallowed, but the worker still returns
{'status': 'success'}unconditionally. That makes dropped outputs look like completed work to the queue consumer and prevents downstream alerting or retry handling from seeing the failure.Possible fix
- for file in files: + had_failures = False + for file in files: @@ - except Exception: + except Exception: + had_failures = True _log_main_exception('Error reprojecting %s', image_file.image_file_url) finally: MAIN_LOGGER.info('Wrote reprojection log to %s', image_log_path) - return False, {'status': 'success'} # No retry under any circumstances + if had_failures: + return False, {'status': 'error', 'status_error': 'reprojection_failed'} + return False, {'status': 'success'} # No retry under any circumstances🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/nav_mosaic_cloud_tasks.py` around lines 254 - 291, The loop currently swallows per-image exceptions (caught in the except block that calls _log_main_exception) but still returns {'status': 'success'}; add failure aggregation by introducing a local flag (e.g., had_error or failed_images list) set inside the except handling for each failed image (referencing IMAGE_LOGGER, _log_main_exception, reproject_one_body, reproject_one_ring, result.save), and after the loop return a non-success response when any image failed (e.g., return False, {'status':'error','failed': <count or list>}), so the caller can detect failures and trigger retries/alerts instead of treating dropped outputs as success.src/nav/reproj/_serialization.py (1)
270-272:⚠️ Potential issue | 🟠 Major
np.savez*()can write to a different filename thanfcpath.upload().Passing
local_pathdirectly here is unsafe when the caller forcesformat_='npz'onfoo.fitsor a suffixless path: NumPy writesfoo.fits.npz, whilefcpath.upload()still targetsfoo.fits. Openlocal_pathin binary mode and pass the file handle tosave_fn()so the written file and uploaded file are guaranteed to be the same object.Verification script
#!/bin/bash python - <<'PY' import numpy as np import tempfile from pathlib import Path with tempfile.TemporaryDirectory() as tmpdir: p = Path(tmpdir) / "example.fits" np.savez(p, x=np.array([1, 2, 3])) print(sorted(child.name for child in Path(tmpdir).iterdir())) PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nav/reproj/_serialization.py` around lines 270 - 272, The code currently calls save_fn(local_path, **arrays) then fcpath.upload(), which can produce mismatched filenames (e.g., writing foo.fits.npz). Instead open local_path in binary write mode and pass the file handle to save_fn (use with open(local_path, "wb") as fh: save_fn(fh, **arrays)) so NumPy writes to the exact file object you will upload; ensure the handle is flushed/closed before calling fcpath.upload(). Keep references to save_fn, local_path, arrays, compress, and fcpath.upload when making this change.src/main/nav_mosaic.py (1)
84-89:⚠️ Potential issue | 🟠 MajorSanitize
results_path_stubbefore building the log filename.
image_file.results_path_stubis concatenated directly intoimage_log_path, so path separators or traversal segments can escape<output_dir>/logsor create attacker-controlled nested paths. Build the log filename from a sanitized basename and verify the resolved path stays under the resolved logs directory beforemkdir()or handler creation.As per coding guidelines, "For file paths, resolve to absolute paths and verify they remain within the expected directory (prevent path traversal)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/nav_mosaic.py` around lines 84 - 89, image_file.results_path_stub is used directly to build image_log_path which allows path traversal or nested attacker-controlled dirs; instead compute a safe filename by taking a sanitized basename of image_file.results_path_stub (e.g., strip path separators, whitespace and replace unsafe chars) and append the timestamp, construct the intended logs_dir (FCPath(output_dir) / 'logs'), then resolve both logs_dir and the final image_log_path and verify image_log_path is inside logs_dir (e.g., using Path.resolve() and is_relative_to or parent checks) before calling image_log_path.parent.mkdir() or returning image_log_handlers(image_log_path, args, DEFAULT_CONFIG); reference symbols: image_file.results_path_stub, image_log_path, image_log_handlers, FCPath, output_dir, DEFAULT_CONFIG.src/nav/ui/mosaic_viewer/ring_window.py (1)
1248-1315:⚠️ Potential issue | 🟠 MajorEnter an explicit empty-data state when the loaded file has zero rows or columns.
_fit_zoom_to_window()now avoids the divide-by-zero, but_load_file()still finishes the normal interactive setup for empty datasets. Downstream handlers like_on_mouse_moved()and_on_ctrl_click()still clip againstdd.n_longitude - 1/dd.n_radii - 1and then index empty arrays. Short-circuit the viewer setup here and clear/disable cursor, EW, and radial-profile interactions for zero-sized files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nav/ui/mosaic_viewer/ring_window.py` around lines 1248 - 1315, The loaded dataset may have zero rows/cols but _load_file currently runs the full interactive setup, leading downstream handlers like _on_mouse_moved and _on_ctrl_click to index empty arrays; after assigning self._current_idx and self._display_data, check dd.n_longitude == 0 or dd.n_radii == 0 and if true short-circuit: clear/disable interactive state (e.g. set self._radial_profile_line = None, clear self._radial_ax, call self._safe_radial_canvas_draw(), hide/disable EW and radial UI via self._rad_wrap, self._cor_wrap, self._chk_rad_profile, self._chk_corot_ew, disable cursor interactions and any profile/ew controls, update statusBar with a clear "empty file" message, refresh file list selection) and return before calling _fit_zoom_to_window/_replot_corot_ew_panel/_apply_ring_display_image that assume non-empty data so downstream _on_mouse_moved/_on_ctrl_click won't index empty arrays.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/experiments/compare_mosaics.py`:
- Around line 123-164: The compatibility check in _compatibility_errors
currently compares shape and resolutions but omits spatial coverage/frame
fields; update _compatibility_errors to also compare BodyMosaicData.lat_range
and lon_range (e.g., obj1.lat_range vs obj2.lat_range and obj1.lon_range vs
obj2.lon_range) when cat1 == 'body', and for ring mosaics compare the longitude
coverage and frame-related metadata (e.g., obj1.longitude_range vs
obj2.longitude_range and whatever frame/id field the ring objects expose such as
obj1.frame or obj1.reference_frame) so that shifted tiles or mismatched frames
are reported as errors before ratio generation (adjust the error messages to
mirror existing style).
- Around line 75-120: The helper _apply_photometry is dropping masks by calling
np.asarray(...) on obj.phase/obj.emission/obj.incidence and forcibly converting
ring incidence with float(...), which loses masked values and crashes for
unknown ring incidence; change these to preserve masked arrays (e.g., use
np.ma.array or avoid np.asarray) so phase/emission/incidence remain
ma.MaskedArray instances before passing to compute_body_display_image and
compute_ring_display_image, and for the ring branch only convert mean_incidence
to a float when it is not masked/None (otherwise pass None through to
compute_ring_display_image which already accepts mean_incidence_deg=None).
Ensure you update references in _apply_photometry (phase_deg, emission_deg,
incidence_deg, and the ring-specific mean_incidence handling) to preserve masks
and allow unknown incidence to pass.
In `@src/main/nav_mosaic_cloud_tasks.py`:
- Around line 179-189: Validate and coerce the task arguments before use: ensure
output_dir_str remains a non-empty str (already checked), then explicitly check
and enforce types for prefix (must be str), fmt/format (must be str and one of
allowed values like 'fits' etc.), overwrite (must be bool or parse a strict set
of string literals 'true'/'false' to bool), no_write_output_files (same bool
handling), and image_name/image_name_override (must be str or None); if any
validation fails return the existing error tuple with a clear status_error
(e.g., 'invalid_prefix_type', 'invalid_format_value', 'invalid_overwrite_type',
etc.), and only after successful validation use prefix, fmt, overwrite,
no_write_output_files, and image_name_override when constructing paths or
branching.
In `@src/nav/reproj/rings.py`:
- Around line 177-205: The function _validate_reproject_zoom_amt currently
accepts 0 which should be rejected; update the checks for both the scalar branch
(instanceof int / np.integer) and the sequence branch (for each z in
enumerate(zoom_amt)) to raise a ValueError when int(z) == 0, while keeping
existing bool/type checks and allowing negative values (spline-order) and
positive integers; ensure the error message clearly states that zoom factors may
be positive ints or negative spline-order values but not zero, and raise the
ValueError from inside the loops/branches that currently set z or append to out
so callers of RingMosaic.reproject get a clear API-boundary error.
- Around line 1210-1267: long_bins_act and long_bins_act_zoom are anchored to
the raw longitude_start instead of the global bin grid; compute their absolute
longitudes from the global bin index base (full_min_lon_bin) instead. Replace
uses of "lon_bins_restr * self._lon_resolution + longitude_start" with
"(full_min_lon_bin + lon_bins_restr) * self._lon_resolution" and likewise
replace "lon_bins_restr_zoom * self._lon_resolution + longitude_start" with
"(full_min_lon_bin + lon_bins_restr_zoom) * self._lon_resolution" so
long_bins_act and long_bins_act_zoom align with the global grid (references:
full_min_lon_bin, lon_bins_restr, lon_bins_restr_zoom, long_bins_act,
long_bins_act_zoom).
In `@src/nav/ui/mosaic_viewer/body_window.py`:
- Around line 234-239: The docstring and __init__ were left at indent level of
_ZoomSync because the class declaration for BodyMosaicWindow was accidentally
removed; reintroduce the class header "class BodyMosaicWindow(QMainWindow):"
immediately before the existing docstring so the docstring and methods like
__init__ are bound to BodyMosaicWindow (not _ZoomSync), and ensure the
indentation of the docstring and subsequent methods matches the class block;
verify imports and any references to BodyMosaicWindow still resolve.
In `@src/nav/ui/mosaic_viewer/common.py`:
- Around line 312-315: Replace the implicit falsy whitespace check with an
explicit empty-string check: compute a stripped_name = orbit_model_name.strip()
(after guarding for None) and use "if orbit_model_name is None or
len(stripped_name) == 0:" to set orbit_model = None, otherwise call
get_orbit_model_by_name(orbit_model_name); update references to use
stripped_name only for the emptiness test so behavior remains the same but
intent is explicit.
- Around line 469-472: The body reprojection code currently always builds
obs_time_tdb by filling an array with float(result.time), which yields
NaN-filled buffers when result.time is not finite; change that to mirror the
ring branch: test np.isfinite(result.time) and if false set obs_time_tdb = None,
otherwise construct the masked array exactly as done now (use image_ma and
float(result.time) to build the MaskedArray). Update the code paths that consume
obs_time_tdb (e.g., body reproj logic and any callers expecting None to mean "no
per-pixel time") so behavior matches the ring branch and aligns with
body_window.py's finite-time guard.
In `@src/nav/ui/mosaic_viewer/tiled_image_widget.py`:
- Around line 694-713: viewport_to_lonlat and _do_paint_projection rely on
display_to_lonlat's validity mask, but the Mollweide projection implementation
returns valid=True for pixels outside the ellipse; update the code that consumes
display_to_lonlat to explicitly invalidate pixels outside the Mollweide ellipse
before using them. Concretely, in viewport_to_lonlat (and the painting routine
_do_paint_projection) compute the Mollweide ellipse test from the projection
parameters created by _make_proj_params (same parameters passed to
display_to_lonlat) and set valid_arr[i]=False for any (vx,vy) that fail the
ellipse test before returning or using lon/lat values; ensure the same fix is
applied to the other duplicate block around the second occurrence (the block at
~1301-1335) so off-ellipse pixels are neither painted nor reported as
on_surface.
---
Outside diff comments:
In `@pyproject.toml`:
- Around line 11-27: Update pyproject.toml to add minimum version bounds for all
unversioned packages: for runtime dependencies add ">=X.Y.Z" style lower bounds
to matplotlib, PyQt6, rms-imgdisp, rms-psfmodel, ruamel.yaml, and scipy in the
dependencies list, and for dev dependencies add minimum versions for coverage,
mypy, pytest, pytest-cov, pytest-profiling, pytest-xdist, pyinstrument, and
ruff; use the project’s supported minimum compatible versions (or known safe
minima) and ensure the format matches the existing entries (e.g.,
"package>=X.Y.Z") so all direct runtime and dev/docs dependencies have explicit
lower bounds.
---
Duplicate comments:
In `@src/main/nav_mosaic_cloud_tasks.py`:
- Around line 254-291: The loop currently swallows per-image exceptions (caught
in the except block that calls _log_main_exception) but still returns {'status':
'success'}; add failure aggregation by introducing a local flag (e.g., had_error
or failed_images list) set inside the except handling for each failed image
(referencing IMAGE_LOGGER, _log_main_exception, reproject_one_body,
reproject_one_ring, result.save), and after the loop return a non-success
response when any image failed (e.g., return False, {'status':'error','failed':
<count or list>}), so the caller can detect failures and trigger retries/alerts
instead of treating dropped outputs as success.
In `@src/main/nav_mosaic.py`:
- Around line 84-89: image_file.results_path_stub is used directly to build
image_log_path which allows path traversal or nested attacker-controlled dirs;
instead compute a safe filename by taking a sanitized basename of
image_file.results_path_stub (e.g., strip path separators, whitespace and
replace unsafe chars) and append the timestamp, construct the intended logs_dir
(FCPath(output_dir) / 'logs'), then resolve both logs_dir and the final
image_log_path and verify image_log_path is inside logs_dir (e.g., using
Path.resolve() and is_relative_to or parent checks) before calling
image_log_path.parent.mkdir() or returning image_log_handlers(image_log_path,
args, DEFAULT_CONFIG); reference symbols: image_file.results_path_stub,
image_log_path, image_log_handlers, FCPath, output_dir, DEFAULT_CONFIG.
In `@src/nav/reproj/_serialization.py`:
- Around line 270-272: The code currently calls save_fn(local_path, **arrays)
then fcpath.upload(), which can produce mismatched filenames (e.g., writing
foo.fits.npz). Instead open local_path in binary write mode and pass the file
handle to save_fn (use with open(local_path, "wb") as fh: save_fn(fh, **arrays))
so NumPy writes to the exact file object you will upload; ensure the handle is
flushed/closed before calling fcpath.upload(). Keep references to save_fn,
local_path, arrays, compress, and fcpath.upload when making this change.
In `@src/nav/ui/mosaic_viewer/ring_window.py`:
- Around line 1248-1315: The loaded dataset may have zero rows/cols but
_load_file currently runs the full interactive setup, leading downstream
handlers like _on_mouse_moved and _on_ctrl_click to index empty arrays; after
assigning self._current_idx and self._display_data, check dd.n_longitude == 0 or
dd.n_radii == 0 and if true short-circuit: clear/disable interactive state (e.g.
set self._radial_profile_line = None, clear self._radial_ax, call
self._safe_radial_canvas_draw(), hide/disable EW and radial UI via
self._rad_wrap, self._cor_wrap, self._chk_rad_profile, self._chk_corot_ew,
disable cursor interactions and any profile/ew controls, update statusBar with a
clear "empty file" message, refresh file list selection) and return before
calling _fit_zoom_to_window/_replot_corot_ew_panel/_apply_ring_display_image
that assume non-empty data so downstream _on_mouse_moved/_on_ctrl_click won't
index empty arrays.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c6d8bfd9-3913-4b5b-a436-68c600ab7dd6
📒 Files selected for processing (14)
docs/developer_guide_reprojection.rstdocs/introduction_overview.rstdocs/user_guide_reprojection.rstpyproject.tomlsrc/experiments/compare_mosaics.pysrc/main/nav_mosaic.pysrc/main/nav_mosaic_cloud_tasks.pysrc/nav/reproj/_serialization.pysrc/nav/reproj/rings.pysrc/nav/ui/mosaic_viewer/body_window.pysrc/nav/ui/mosaic_viewer/common.pysrc/nav/ui/mosaic_viewer/ring_window.pysrc/nav/ui/mosaic_viewer/tiled_image_widget.pysrc/reproj_cli/offsets.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Add a full reprojection and mosaic workflow for planetary image data, an interactive PyQt6 mosaic viewer, and CLI entry points so navigated Cassini, Galileo, Voyager, and New Horizons imagery can be reprojected, merged into body and ring mosaics, inspected with multiple projections and photometric display modes, and driven from the command line (including optional cloud-style batching).
Closes #
Changes
nav.reproj(src/nav/reproj/)bodies.pyandrings.py—BodyMosaic/RingMosaicwith per-image*ReprojResult/*MosaicData: reprojection, sparse, bounded, and full storage layouts, batched insertion, merge strategies (BEST_RESOLUTION,MOST_COVERAGE_THEN_RESOLUTION), contributing-image tracking, and save/load.photometric_model.py— Lambert, Lommel-Seeliger, and Minnaert models withcorrect/uncorrectandphotometric_model_from_name.cartographic_model.py— cartographic body geometry for reprojection.ring_orbit_model.py— ring orbit frame handling (longitude shift, radius and zoom ranges)._serialization.py— FITS / NPZ persistence with dtype verification (_dtype_matches_declared,verify_dtype)._context_managers.py— shared small context helpers.reproj_cli(src/reproj_cli/)args.py— shared argparse helpers (add_common_env_args,add_common_output_args,add_body_args,add_ring_args).factories.py—build_body_mosaic/build_ring_mosaicandparse_zoom_arg.offsets.py— load(dv, du)fromnav_offsetmetadata and apply viaOffsetFOV.paths.py— deterministic per-image and mosaic output paths with subject-name sanitization.reproject.py—reproject_one_body/reproject_one_ring, including validation of--longitude-range/--radius-rangelength-2 tuples.nav.ui.mosaic_viewertiled_image_widget.py— tiled renderer for large mosaics; ring (radius vs longitude) and body (latitude vs longitude) geometries; Polar N/S, Mollweide, and orthographic sphere projections.body_window.py/ring_window.py— viewer shells with stretch and gamma, projection switching, photometric modes, graticules, and per-feature annotations. UI improvements: parallels/meridians checkboxes relocated to the header row adjacent to the Projection dropdown; the separate Overlays panel removed; image file list now fills the full available height; effective-resolution label width increased to prevent truncation; metadata grid column-stretch fix to eliminate gaps between labels and values; Color By box gains an Alpha slider (0–100 %) to blend tint transparency in both ring and body viewers.graticule.py,projections.py,sphere_render.py— coordinate math, graticule polylines, and RGB software rendering for the sphere view (including safe buffer lifetime forQImage).photometric_display.py—as_saved,intrinsic, and photometric-model display modes.common.py,matplotlib_qt.py— shared stretch, zoom, pan helpers and typedFigureCanvasQTAggwrappers.Entry points and automation
src/main/nav_mosaic.py—nav_mosaic {rings|body}two-pass workflow (reproject pass, mosaic pass), nav-offset application, per-image logging,--skip-reproject,--skip-mosaic,--dry-run,--overwrite.--output-cloud-tasks-fileembeds"mode"in each task payload.src/main/nav_mosaic_cloud_tasks.py— Cloud Tasks reprojection worker; reads the mode ("rings"or"body") per-task fromtask_data['mode'], allowing a singlenav_mosaic_cloud_tasksworker to drain a queue containing both ring and body tasks.src/main/nav_mosaic_display.py— opens the mosaic viewer on a saved mosaic or reprojection product.Experiments (
src/experiments/)compare_mosaics.py— CLI tool for comparing two ring or body mosaic/reprojection files. Applies an optional photometric adjustment (as_saved,uncorrected/intrinsic,lambert,lommel_seeliger,minnaert), checks compatibility (same body, grid dimensions and resolutions), and prints a three-column statistics table (File 1, File 2, Ratio) with N valid, min, P10, P25, median, mean, std dev, P75, P90, max — all computed over the pixel overlap between the two images, or over each file's full valid set with--analyze-all-pixels(which also downgrades incompatibilities to warnings). Optionally saves the ratio as a new mosaic file readable bynav_mosaic_displayvia--output-ratio-file.Shared UI and support
nav.ui.common— stretch helpers used by the manual navigation dialog and mosaic viewer; linear/gamma stretch tolerates equal or inverted black/white by nudging the white point to keep the scale denominator valid (avoids hard failures when sliders coincide).nav.support— removes deprecatedcorrelate_old; routine package exports adjusted. Touch-ups toimage,misc, andnav_basewhere reprojection and UI paths share helpers.Datasets, obs, and bundles
nav_create_bundle,nav_backplanes, backplane writer/reader, and related mains adjusted for logging and compatibility with the expanded dependency surface.Documentation and repo hygiene
docs/developer_guide_reprojection.rstanddocs/user_guide_reprojection.rst; extensions to backplanes, navigation, best practices, class hierarchy, configuration, building docs, simulated images, introduction overview, and correlate developer notes where they intersect navigation products. Cloud-tasks documentation covers the unifiednav_mosaic_cloud_tasksworker and per-taskmodefield.api_reprojand extendedapi_ui.README.md— installation and usage examples for mosaic and viewer workflows.scripts/read_docs.sh— local documentation build helper aligned with CI..readthedocs.yaml— Read the Docs image and dependency install path updates.pyproject.toml— optional dependencies and console script registration fornav_mosaic,nav_mosaic_display,nav_mosaic_cloud_tasks, and related tooling.Tests
tests/nav/reproj/(test_bodies,test_rings,test_cartographic_model,test_serialization_fits,test_shared_modules),tests/nav/ui/(test_common,test_graticule,test_projections,test_sphere_render),tests/reproj_cli/(test_paths,test_factories).tests/conftest.py,tests/nav/nav_model/test_nav_model_rings.py,tests/nav/support/test_upsampled_dft.py, and related fixtures where geometry or dtypes changed.Type of Change
Testing
./scripts/run-all-checks.sh -spasses:ruff check,ruff format --check,mypy,pytest(with coverage), Sphinx (SPHINXOPTS=-W), and PyMarkdown.Manual verification:
nav_mosaic rings/nav_mosaic bodyend-to-end (reproject pass, mosaic pass,--dry-run,--skip-*,--overwrite); inspection in the mosaic viewer for ring and body products across projections, photometric modes, graticules, and annotations; smoke check ofnav_mosaic_displayon saved outputs;compare_mosaics.pyrun against identical and differing body mosaics.Potential Impacts
nav.reproj,nav.ui.mosaic_viewer,reproj_cli, and newnav_mosaic*console scripts. Existing navigation and offset entry points keep prior behavior aside from targeted logging and typing cleanups.nav.support.correlate_oldbreaks any external import of that module; usenav.support.correlateinstead.nav_mosaic_cloud_tasksis the sole cloud-tasks entry point for mosaic reprojection; task payloads require a"mode"field ("rings"or"body") in each task'sdataobject.rms-oops,rms-starcat, orrms-psfmodelin this repository.Checklist
ruff check,ruff format)mypypassesNotes
Summary by CodeRabbit
New Features
Documentation
Chores
Bug Fixes