Skip to content

metadata oops updates #190

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jnspitale wants to merge 4 commits into
base: main
Choose a base branch
from
Open

metadata oops updates #190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
feb5817
safety push
jnspitale Dec 10, 2025
aff24c2
Merge branch 'main' into jns-metadata-updates
jnspitale Dec 10, 2025
0f6090b
coderabbit fixes
jnspitale Dec 11, 2025
8e26cde
Merge branch 'jns-metadata-updates' of https://github.com/SETI/rms-oo…
jnspitale Dec 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions oops/backplane/limb.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ def limb_altitude(self, event_key, zmin=None, zmax=None, scaled=False):
radius = body.surface.radii.max()
if zmin is not None:
zmin = zmin * radius
if zmin is not None:
if zmax is not None:
zmax = zmax * radius

key = ('limb_altitude', event_key, zmin, zmax)
Expand Down Expand Up @@ -159,7 +159,7 @@ def limb_latitude(self, event_key, lat_type='centric'):
return self.register_backplane(key, latitude)

#===============================================================================
def limb_clock_angle(self, event_key):
def limb_clock_angle(self, event_key, zmin=None, zmax=None, scaled=False):
Copy link

@coderabbitai coderabbitai bot Dec 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Suggested change
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.

Copy link
Collaborator

@markshowalter markshowalter Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@coderabbitai coderabbitai bot Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

"""Angular location around the limb, measured clockwise from the projected
north pole.

Expand All @@ -168,8 +168,16 @@ def limb_clock_angle(self, event_key):
limb_altitude backplane key, in which case this
backplane inherits the mask of the given backplane
array.
zmin lower limit on altitude; lower values are masked.
zmax upper limit on altitude.
scaled if True, zmin and zmax are in units of the maximum body radius.
"""

# Get the altitude backplane
altitude_key = ('limb_altitude', event_key, zmin, zmax)
Copy link

@coderabbitai coderabbitai bot Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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.

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')
Expand All @@ -194,8 +202,13 @@ def limb_clock_angle(self, event_key):
event = polar_surface.apply_coords_to_event(event, obs=self.obs_event,
axes=2,
derivs=self.ALL_DERIVS)
clock_angle = event.coord2

# copy altitude mask
if np.any(altitude.mask):
clock_angle = clock_angle.remask_or(altitude.mask)

return self.register_backplane(key, event.coord2)
return self.register_backplane(key, clock_angle)

################################################################################

Expand Down
10 changes: 7 additions & 3 deletions oops/hosts/galileo/ssi/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
import pdsparser
import oops

from oops.hosts import pds3
from oops.hosts.galileo import Galileo

from filecache import FCPath
Expand Down Expand Up @@ -42,7 +41,7 @@ def from_file(filespec,
filespec = FCPath(filespec)

# Load the PDS label
label = pds3.get_label(filespec)
label = pdsparser.PdsLabel.from_file(filespec).as_dict()
Copy link
Collaborator

@markshowalter markshowalter Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@jnspitale jnspitale Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@rfrenchseti rfrenchseti Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# Load the data array
local_path = filespec.retrieve()
Expand Down Expand Up @@ -211,6 +210,8 @@ def __init__(self, meta_dict):
self.target = meta_dict['TARGET_NAME']

# Telemetry mode
if 'TELEMETRY_FORMAT_ID' not in meta_dict:
meta_dict['TELEMETRY_FORMAT_ID'] = 'NONE'
self.mode = meta_dict['TELEMETRY_FORMAT_ID']

# Window
Expand Down Expand Up @@ -366,9 +367,12 @@ def initialize(planets=None, asof=None,
SSI.fovs['HCM'] = fov_full # Inference based on inspection
# hmmm, actually C0248807700R.img is 800x200
# maybe this is just a cropped full fov
SSI.fovs['NONE'] = fov_full # Inference based on inspection

# Construct the SpiceFrame
_ = oops.frame.SpiceFrame("GLL_SCAN_PLATFORM")
# SSI images are spaced as closely as 1 unit in the file name, which
# corresponds to 80 clock ticks. Therefore, we use a tolerance of +/-40
_ = oops.frame.SpiceType1Frame("GLL_SCAN_PLATFORM", -77, 40)

# Load kernels
Galileo.load_kernels()
Expand Down