metadata oops updates #190
Conversation
WalkthroughTwo files updated: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
oops/backplane/limb.py (1)
177-213: Critical caching bug: backplane key missing altitude parameters.The backplane key at line 186 is defined as
('limb_clock_angle', event_key)but does not include thezmin,zmax, andscaledparameters. This means that different calls with different altitude constraints will incorrectly return the same cached backplane, leading to incorrect masking.Additionally,
altitude_keyat line 178 is assigned but never used.Apply this diff to fix the caching bug and remove the unused variable:
# Get the altitude backplane - altitude_key = ('limb_altitude', event_key, zmin, zmax) altitude = limb_altitude(self, event_key, zmin=zmin, zmax=zmax, scaled=scaled) # Create the clock angle backplane (event_key, backplane_key) = self._event_and_backplane_keys(event_key, LIMB_BACKPLANES, default='LIMB') - key = ('limb_clock_angle', event_key) + key = ('limb_clock_angle', event_key, zmin, zmax, scaled) if backplane_key: return self._remasked_backplane(key, backplane_key) # If this backplane array is already defined, return it if key in self.backplanes: return self.get_backplane(key) # Make sure the limb event is defined - default_key = ('limb_clock_angle', event_key) + default_key = ('limb_clock_angle', event_key, None, None, False) if default_key not in self.backplanes: self._fill_limb_intercepts(event_key)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
oops/backplane/limb.py(4 hunks)oops/hosts/galileo/ssi/__init__.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.14.8)
oops/hosts/galileo/ssi/__init__.py
215-215: Test for membership should be not in
Convert to not in
(E713)
oops/backplane/limb.py
163-163: Boolean default positional argument in function definition
(FBT002)
178-178: Local variable altitude_key is assigned to but never used
Remove assignment to unused variable altitude_key
(F841)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Test oops (self-hosted-linux, 3.9)
- GitHub Check: Test oops (self-hosted-linux, 3.10)
- GitHub Check: Test oops (self-hosted-linux, 3.11)
- GitHub Check: Test oops (self-hosted-macos, 3.13)
- GitHub Check: Test oops (self-hosted-linux, 3.12)
- GitHub Check: Test oops (self-hosted-macos, 3.12)
- GitHub Check: Test oops (self-hosted-linux, 3.13)
- GitHub Check: Test oops (self-hosted-macos, 3.11)
🔇 Additional comments (1)
oops/hosts/galileo/ssi/__init__.py (1)
372-372: LGTM!The
NONEFOV entry properly supports the defaultTELEMETRY_FORMAT_IDvalue introduced in the metadata handling (line 216), ensuring a consistent fallback behavior.
oops/backplane/limb.py
Outdated
| ### use self._remasked_backplane | ||
| # copy altitude mask | ||
| if np.any(altitude.mask): | ||
| clock_angle = clock_angle.remask_or(altitude.mask) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using the _remasked_backplane pattern.
The comment at line 208 suggests using self._remasked_backplane, which is the pattern used in limb_longitude and limb_latitude (lines 120, 152). This would be more consistent with the existing codebase.
Do you want me to generate a refactored implementation that uses the _remasked_backplane pattern consistently?
oops/hosts/galileo/ssi/__init__.py
Outdated
| if not 'TELEMETRY_FORMAT_ID' in meta_dict: | ||
| meta_dict['TELEMETRY_FORMAT_ID'] = 'NONE' | ||
| self.mode = meta_dict['TELEMETRY_FORMAT_ID'] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good defensive programming; consider style improvement.
The default value for TELEMETRY_FORMAT_ID properly prevents potential KeyError exceptions and aligns with the new NONE entry in the FOV mapping (line 372).
For consistency with Python style guidelines, consider this minor refactoring:
- if not 'TELEMETRY_FORMAT_ID' in meta_dict:
+ if 'TELEMETRY_FORMAT_ID' not in meta_dict:
meta_dict['TELEMETRY_FORMAT_ID'] = 'NONE'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not 'TELEMETRY_FORMAT_ID' in meta_dict: | |
| meta_dict['TELEMETRY_FORMAT_ID'] = 'NONE' | |
| self.mode = meta_dict['TELEMETRY_FORMAT_ID'] | |
| if 'TELEMETRY_FORMAT_ID' not in meta_dict: | |
| meta_dict['TELEMETRY_FORMAT_ID'] = 'NONE' | |
| self.mode = meta_dict['TELEMETRY_FORMAT_ID'] |
🧰 Tools
🪛 Ruff (0.14.8)
215-215: Test for membership should be not in
Convert to not in
(E713)
🤖 Prompt for AI Agents
In oops/hosts/galileo/ssi/__init__.py around lines 215 to 217, the code
defensively sets TELEMETRY_FORMAT_ID via an if-check then assigns self.mode;
refactor to a single line using dict.get to follow Python style guidelines and
reduce verbosity: replace the conditional block with a direct assignment like
self.mode = meta_dict.get('TELEMETRY_FORMAT_ID', 'NONE') so the default is
applied inline and the behavior remains the same.
… into jns-metadata-updates
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
oops/backplane/limb.py (1)
162-211: Critical caching bug: altitude parameters missing from backplane key.The caching key at line 185 is
('limb_clock_angle', event_key), which doesn't include the altitude parameters (zmin,zmax,scaled). This causes incorrect behavior when the function is called multiple times with the sameevent_keybut different altitude ranges—the second call will return the cached result from the first call instead of computing a newly masked clock angle.Compare with
limb_altitudeat line 36:key = ('limb_altitude', event_key, zmin, zmax), which correctly includes the altitude parameters in the caching key.Apply this diff to include altitude parameters in the key:
- # Create the clock angle backplane (event_key, backplane_key) = self._event_and_backplane_keys(event_key, LIMB_BACKPLANES, default='LIMB') - key = ('limb_clock_angle', event_key) + key = ('limb_clock_angle', event_key, zmin, zmax) if backplane_key: return self._remasked_backplane(key, backplane_key)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
oops/backplane/limb.py(4 hunks)oops/hosts/galileo/ssi/__init__.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
oops/hosts/galileo/ssi/__init__.py (2)
oops/hosts/voyager/iss.py (1)
from_file(22-190)oops/frame/spicetype1frame.py (1)
SpiceType1Frame(14-194)
🪛 Ruff (0.14.8)
oops/backplane/limb.py
162-162: Boolean default positional argument in function definition
(FBT002)
177-177: Local variable altitude_key is assigned to but never used
Remove assignment to unused variable altitude_key
(F841)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Test oops (self-hosted-linux, 3.12)
- GitHub Check: Test oops (self-hosted-linux, 3.13)
- GitHub Check: Test oops (self-hosted-linux, 3.9)
- GitHub Check: Test oops (self-hosted-macos, 3.11)
- GitHub Check: Test oops (self-hosted-macos, 3.13)
- GitHub Check: Test oops (self-hosted-macos, 3.12)
- GitHub Check: Test oops (self-hosted-linux, 3.11)
- GitHub Check: Test oops (self-hosted-linux, 3.10)
- GitHub Check: Test oops (self-hosted-windows, 3.10)
🔇 Additional comments (5)
oops/backplane/limb.py (1)
33-34: LGTM! The zmax scaling issue has been resolved.The scaling of
zmaxis now independent ofzmin, correctly addressing the critical issue raised in the previous review.oops/hosts/galileo/ssi/__init__.py (4)
213-214: LGTM! Pythonic style applied correctly.The defensive default for
TELEMETRY_FORMAT_IDnow uses the Pythonicnot inoperator, and correctly aligns with the newNONEFOV entry added at line 370.
370-370: Good defensive programming.Adding the
NONEentry to the FOV mapping provides a sensible fallback for cases whereTELEMETRY_FORMAT_IDis missing or unrecognized, complementing the default handling added at lines 213-214.
373-375: Code is correct and appropriately documented.The switch to
SpiceType1Framewith explicit spacecraft ID (-77) and tick tolerance (40) is well-justified. The frame automatically registers itself viaself.register()called during__init__(), so assigning to_is the appropriate idiom for side-effect-only instantiation. The tick tolerance of 40 aligns with the documented SSI image spacing: 1 unit in filename = 80 clock ticks, therefore ±40 tick tolerance.
44-44: No compatibility concern—both approaches use pdsparser.The label loading change to
pdsparser.PdsLabel.from_file().as_dict()is functionally equivalent to the previouspds3.get_label()approach, as both use the same underlying pdsparser library (seeoops/hosts/pds3.pyline 67). The keysEXPOSURE_DURATION,FILTER_NAME,IMAGE_TIME, andTARGET_NAMEare standard top-level PDS3 label keys used consistently across Voyager, Cassini, Juno, and Galileo instruments in this codebase. The existing defensive check forTELEMETRY_FORMAT_ID(line 213) confirms awareness of optional keys. No structural incompatibility exists.
|
|
||
| #=============================================================================== | ||
| def limb_clock_angle(self, event_key): | ||
| def limb_clock_angle(self, event_key, zmin=None, zmax=None, scaled=False): |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider keyword-only parameters for boolean flags.
The scaled parameter is a boolean flag with a default value. Python style guidelines suggest making such parameters keyword-only to improve call-site clarity and prevent accidental positional boolean arguments.
Apply this diff:
-def limb_clock_angle(self, event_key, zmin=None, zmax=None, scaled=False):
+def limb_clock_angle(self, event_key, zmin=None, zmax=None, *, scaled=False):This change would also apply to limb_altitude at line 16 for consistency:
-def limb_altitude(self, event_key, zmin=None, zmax=None, scaled=False):
+def limb_altitude(self, event_key, zmin=None, zmax=None, *, scaled=False):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def limb_clock_angle(self, event_key, zmin=None, zmax=None, scaled=False): | |
| def limb_clock_angle(self, event_key, zmin=None, zmax=None, *, scaled=False): |
🧰 Tools
🪛 Ruff (0.14.8)
162-162: Boolean default positional argument in function definition
(FBT002)
🤖 Prompt for AI Agents
In oops/backplane/limb.py around line 162 (and similarly at line 16 for
limb_altitude), the boolean flag parameter scaled should be made keyword-only to
avoid accidental positional use; update the function signatures to place a bare
* before scaled (e.g., include zmin=None, zmax=None, *, scaled=False) so callers
must specify scaled=... and adjust any internal or external calls if they
currently pass scaled positionally.
There was a problem hiding this comment.
CodeRabbit's proposed change will not work, because evaluate needs to be able to take a backplane key, which is a tuple, and pass its values as inputs to this function. As a result, backplane functions can only take positional arguments.
There was a problem hiding this comment.
@markshowalter, thank you for the clarification! I understand now—since evaluate unpacks backplane keys (tuples) as positional arguments when calling these functions, keyword-only parameters would break that mechanism. I've noted this constraint for future reviews.
✏️ Learnings added
Learnt from: markshowalter
Repo: SETI/rms-oops PR: 190
File: oops/backplane/limb.py:162-162
Timestamp: 2025-12-12T20:08:16.037Z
Learning: In the oops/backplane system, backplane functions must accept only positional arguments because the `evaluate` function takes backplane keys (which are tuples) and passes their values as positional arguments to backplane functions. Keyword-only parameters (using * in the signature) should not be suggested for backplane functions.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| """ | ||
|
|
||
| # Get the altitude backplane | ||
| altitude_key = ('limb_altitude', event_key, zmin, zmax) |
There was a problem hiding this comment.
Remove unused variable.
The altitude_key variable is assigned but never used in the function.
Apply this diff:
- # Get the altitude backplane
- altitude_key = ('limb_altitude', event_key, zmin, zmax)
- altitude = limb_altitude(self, event_key, zmin=zmin, zmax=zmax, scaled=scaled)
+ # Get the altitude backplane
+ altitude = limb_altitude(self, event_key, zmin=zmin, zmax=zmax, scaled=scaled)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| altitude_key = ('limb_altitude', event_key, zmin, zmax) | |
| # Get the altitude backplane | |
| altitude = limb_altitude(self, event_key, zmin=zmin, zmax=zmax, scaled=scaled) |
🧰 Tools
🪛 Ruff (0.14.8)
177-177: Local variable altitude_key is assigned to but never used
Remove assignment to unused variable altitude_key
(F841)
🤖 Prompt for AI Agents
In oops/backplane/limb.py around line 177 the local variable `altitude_key =
('limb_altitude', event_key, zmin, zmax)` is assigned but never used; remove
this unused assignment (delete the line) to avoid dead code and any linter
warnings, and run tests/type checks to ensure nothing else relied on it.
markshowalter
left a comment
markshowalter
left a comment
There was a problem hiding this comment.
There's a long comment about the changes to limb_clock_angle(), which I think may turn out to be unnecessary. Let me know if you have any questions about what I wrote.
|
|
||
| #=============================================================================== | ||
| def limb_clock_angle(self, event_key): | ||
| def limb_clock_angle(self, event_key, zmin=None, zmax=None, scaled=False): |
There was a problem hiding this comment.
CodeRabbit's proposed change will not work, because evaluate needs to be able to take a backplane key, which is a tuple, and pass its values as inputs to this function. As a result, backplane functions can only take positional arguments.
|
|
||
| # Load the PDS label | ||
| label = pds3.get_label(filespec) | ||
| label = pdsparser.PdsLabel.from_file(filespec).as_dict() |
There was a problem hiding this comment.
You should change this to:
local_path = filespec.retrieve()
label = pdsparser.Pds3Label(local_path)
I think this is cleaner because it makes the retrieve() explicit and now it's clear that both pdsparser and vicar are working with a local file.
Also,
- "Pds3Label" is now the preferred class name, although "PdsLabel" is an alias.
- The constructor knows how to handle file paths, so
from_file()is not needed. - The Pds3Label implements the dictionary interface, so
as_dict()isn't really needed.
There was a problem hiding this comment.
I disagree. The code is indeed bad here, as the very next line retrieves the file into local_path prior to calling vicar.VicarImage.from_file(), but I would argue for removing that and not even referencing a local file.
I view filecache as allowing our code to access local and remote files seamlessly, and the only reason for explicitly retrieving a file in the calling code is because it is intended to be used with a package that doesn't support filecache.
We could have simply converted file specs to FCPaths and the original code would have worked fine if the Vicar and various pds modules had supported filecache at the time.
I would prefer to remove all explicit retrieve() operations that are no longer necessary.
There was a problem hiding this comment.
I agree. Except in unusual circumstances the goal is to never have a "retrieve" in any code. It's always handled by the libraries. Otherwise there's no point in having made all of our libraries FCPath-compatible.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.