Add custom C-matrix support via generic Observation.get_cmatrix/set_cmatrix

[jnspitale]

Summary

• hosts/cassini/iss.py from_file() accepts a caller-supplied cmatrix= — the spice-convention C-matrix (J2000 → CASSINI_ISS_<camera>, exactly as returned by cspyce.pxform() or recorded in a CK) — plus an optional frame_id=. A custom-cmatrix load never loads a CK, never builds the SPICE camera frames (now built lazily by ISS.define_camera_frames()), and never reports CKs in spice_kernels.
• Generic Observation.get_cmatrix(uv=None, time=None) and Observation.set_cmatrix(cmatrix, frame_id=None) on the base class. Both are driven by a single host class attribute, CMATRIX_ROTATION (the spice-frame → oops-frame convention rotation, reached through the observation's host subfield); the inverse direction is derived by transposition. No host-specific set/get methods, and nothing frame-shaped is stored on the observation — get_cmatrix derives the C-matrix from self.frame at call time.
• set_cmatrix validates its input: the matrix must be a proper floating-point rotation. Non-finite values, boolean/integer data, reflections, scalings, and non-orthogonal matrices raise ValueError at the boundary.
• set_cmatrix(frame_id=) only overrides frame registrations it made itself; an ID already held by any other frame (e.g. CASSINI_ISS_NAC, J2000) raises instead of silently replacing its primary definition. frame_id=None (default) gives each observation its own unregistered frame; re-using a set_cmatrix-issued ID re-points every observation sharing that registered frame, which is the point of giving it an ID.
• oops/frame/cmatrix.py Cmatrix gains an override= option so an owned registration can be cleanly replaced.

Breaking changes

• CASSINI_ISS_NAC_FLIPPED / CASSINI_ISS_WAC_FLIPPED are renamed to *_SPICE.
• ISS.initialize() alone no longer registers CASSINI_ISS_NAC/CASSINI_ISS_WAC; they appear after the first plain (SPICE-pointed) from_file(), which calls define_camera_frames().
• The offset_wac initialize option is removed: it made the WAC pointing a composite (Navigation offset on top of the SPICE chain), so get_cmatrix could not agree with pxform and the meaning of from_file(cmatrix=) depended on an initialize-time flag.

Review

Addresses the independent verification review (Custom C-Matrix Verification, R. French, 2026-08-14): F1 rotation validation, F2 frame-registry protection, F3 offset_wac removal, F5/F6 docstring clarifications (stale derived geometry; no CK loaded so pxform against the SPICE camera frame fails for that epoch). F4 (enabling the other hosts: Galileo SSI, Voyager ISS, NH LORRI) is deliberately deferred to #204 — this branch is the template.

Test plan

• tests/hosts/cassini/iss/test_iss.py — 18 tests: round-trip vs pxform, per-observation frame isolation, no leakage into plain loads, frame_id reuse/collision, C-matrix validation, CK loading and spice_kernels reporting
• tests/frame/test_cmatrix.py
• tests/observation suite

🤖 Generated with Claude Code

https://claude.ai/code/session_015Wg9zMfz6FNEotebnS1hmm

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Changes

The Cassini ISS host now accepts custom SPICE C-matrices, converts them into observation frames, and creates camera frames lazily. C-matrix registration supports controlled overrides. New tests cover conversions, frame isolation, CK loading, registration, and reset behavior.

Cassini ISS C-matrix flow

Layer / File(s)	Summary
C-matrix conversion and registration oops/frame/cmatrix.py, oops/observation/observation_.py	Observation converts between SPICE and observation-frame conventions. Cmatrix supports optional override registration.
ISS camera frame setup oops/hosts/cassini/iss.py	ISS.from_file accepts custom matrices and frame IDs. CK loading and camera-frame construction now depend on the pointing source.
ISS C-matrix validation tests/hosts/cassini/iss/*	Tests cover matrix round trips, custom and registered frames, CK behavior, global-frame preservation, reset cleanup, and test-runner registration.

Estimated code review effort: 4 (Complex) | ~45 minutes

Mergeability Score: 🟡 Moderate · up to 20230

Repeated custom C-matrix assignment can leave stale camera pointing, potentially producing incorrectly oriented observations, while observations missing required host metadata fail unclearly. Merge should wait for explicit replacement handling and clearer validation.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main changes: custom C-matrix support and a new observation-level C-matrix getter.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@oops/hosts/cassini/iss.py`:
- Around line 497-501: The rel calculation in the camera-frame setup must use
immutable SPICE-only frame references rather than mutable CASSINI_ISS_* frames
that set_cmatrix can replace. Update define_camera_frames and the surrounding
frame setup to retain and use dedicated SPICE inter-camera frames for rel, and
add a regression test covering a single-camera override followed by a
mapped-camera load.

In `@tests/observation/test_snapshot.py`:
- Around line 135-139: Extend the snapshot observation test around cmatrix() so
it uses a time-dependent frame and observation whose midtime_at_uv() returns
different values for different UV coordinates. Assert that selecting distinct
UVs and times produces the corresponding transformed matrices, ensuring
cmatrix() actually uses both uv and time rather than returning the fixed
Cmatrix.

🪄 Autofix

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 Plus

Run ID: 05dd31e2-5483-41dc-b1c7-b30af5a8aac7

📥 Commits

Reviewing files that changed from the base of the PR and between 5b1d0d4 and cdad9ca.

📒 Files selected for processing (4)

• oops/frame/cmatrix.py
• oops/hosts/cassini/iss.py
• oops/observation/observation_.py
• tests/observation/test_snapshot.py

[jnspitale]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@oops/hosts/cassini/iss.py`:
- Around line 597-608: Update the mapped-camera handling around
define_camera_frames and the returned frame ID so each mapped observation
receives a separate fixed frame rather than the mutable global CASSINI_ISS
camera frame. Keep the global frames only for mapped-camera registration,
preserve the observation’s frame across subsequent plain and mapped loads, and
add a regression test that reads the first mapped observation after both a plain
load and a second mapped load.

In `@tests/hosts/cassini/iss/test_iss.py`:
- Around line 66-80: Use a consistent evaluation time for all C-matrix
comparisons in tests/hosts/cassini/iss/test_iss.py: in lines 66-80, compare
against baseline.cmatrix(time=baseline.tstart), and in lines 97-104, pass
time=baseline.tstart to both baseline.cmatrix() calls. Keep the existing
round-trip and boundary-rotation assertions unchanged.

🪄 Autofix

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 Plus

Run ID: 30a38d76-68e1-45c1-b1ae-bc8aad0c3fa1

📥 Commits

Reviewing files that changed from the base of the PR and between 4ab0943 and d92a221.

📒 Files selected for processing (3)

• oops/hosts/cassini/iss.py
• oops/observation/observation_.py
• tests/hosts/cassini/iss/test_iss.py

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@oops/hosts/cassini/iss.py`:
- Line 621: Add the return annotation -> oops.Matrix3 to ISS.get_cmatrix, or
explicitly exclude this method from ANN205 if this module intentionally omits
return annotations. Preserve the existing method behavior and signature
parameters.

🪄 Autofix

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 Plus

Run ID: dee3c0fe-c3da-4dcc-bad5-8df0c442da67

📥 Commits

Reviewing files that changed from the base of the PR and between d92a221 and 5ef6ce7.

📒 Files selected for processing (2)

• oops/hosts/cassini/iss.py
• tests/hosts/cassini/iss/test_iss.py

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@oops/observation/observation_.py`:
- Around line 482-487: Update get_cmatrix() and set_cmatrix() to explicitly
validate that the observation has a host subfield before accessing
host.CMATRIX_ROTATION. Raise a clear error identifying the missing convention
rotation when host is absent, while preserving the existing behavior when the
host is present.
- Around line 517-524: Update set_cmatrix to accept an override=False parameter
and replace the duplicated Cmatrix construction branches with one
Cmatrix(attitude, frame_id=frame_id, override=override) call. Document that
override=True replaces the existing primary definition for a duplicate frame_id,
while preserving unregistered-frame behavior when frame_id is None.

🪄 Autofix

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 Plus

Run ID: 1da334a4-806c-418f-9170-cb7f3b10316d

📥 Commits

Reviewing files that changed from the base of the PR and between 5ef6ce7 and 202305b.

📒 Files selected for processing (3)

• oops/hosts/cassini/iss.py
• oops/observation/observation_.py
• tests/hosts/cassini/iss/test_iss.py

[astrocfi]

See report: https://claude.ai/code/artifact/ae1c9373-6d52-4efa-878d-2943637ef4f4

[rfrenchseti]

One forward-compatibility note on Observation.set_cmatrix(), since #203 is in flight against the same base.

The frame_id=None branch is documented as attaching "a fresh unregistered frame owned by the observation, so loading other images never disturbs its pointing". That ownership property does hold as this PR stands — I checked that constructing many unregistered Cmatrix frames leaves Frame.FRAME_CACHE and Frame.WAYFRAME_REGISTRY flat at 2 entries, and that two frames built from an equal-valued matrix are distinct objects.

But it isn't a property that Cmatrix.__init__ or Frame.register() actually promises. It is emergent from how the current registry treats a frame_id of None, and #203 changes exactly that: under its registry rewrite Frame._register() inserts every frame into a per-subclass _WAYFRAMES dict regardless of frame_id, and equal-valued unregistered frames dedup to a single shared wayframe object. On that branch Cmatrix(m)._wayframe is Cmatrix(m)._wayframe is True, and each construction is retained globally for the life of the process.

If #203 lands after this, the docstring's guarantee becomes false without anything in either PR failing — the frame is no longer owned by the observation, and it is no longer isolated from global state. Two observations handed equal-valued C-matrices would share a frame object.

Either of these would be enough to keep that from happening quietly:

• Document the weaker, implementation-independent property — that the frame is simply not registered under a frame ID — rather than ownership or isolation from other loads.
• Add a test that pins the property you actually want: that two observations given equal-valued C-matrices don't end up sharing a frame object, and that set_cmatrix(frame_id=None) leaves the frame registry unchanged. That turns a silent semantic regression into a failing test whenever the registry internals move.

Worth coordinating with #203 either way, since the two PRs are independent branches off main and whichever merges second inherits the interaction.

[rfrenchseti]

See earlier comments

[jnspitale]

Good catch — agreed that the isolation property was emergent rather than promised anywhere. Addressed by pinning it as a test rather than weakening the docstring, since per-observation ownership is the feature this argument exists for, not an accident we're willing to lose:

test_default_frames_are_distinct_and_stay_out_of_registry now asserts exactly the two properties you suggested — observations given equal-valued C-matrices get distinct frame objects, and a set_cmatrix(frame_id=None) load leaves Frame.WAYFRAME_REGISTRY and Frame.FRAME_CACHE at their prior sizes (measured after a warm load+get_cmatrix cycle, so lazy one-time registrations don't confound it). Under the #203 registry semantics you describe, both assertions fail, so whichever branch merges second now hits a failing test at rebase instead of a silent regression.

The retention/dedup question itself is #203's design call — raised there with a pointer back to this contract.

[jnspitale]

This is ready for re-review. Main concern is interaction with 203.

[markshowalter]

We're going to need to discuss this. My first issue is that the override option in Cmatrix needs to be implemented via the new Fittable/Mutable API. You can't just replace the definition of an object without notifying any object on which it depends. This is what the new API handles automatically. I did not define class Cmatrix to be Fittable, and recommend that we don't. Instead, a user should be able to define a Navigation frame that operates on the Cmatrix, and then adjust the Navigation. A lot of this needs to be built into the planned rewrite of the host modules. Until then, we can discuss what is specifically needed as an interim fix.

Maybe we can just define every Cassini frame as a Navigation atop a Cmatrix, because a Navigation object has very little overhead, and then you could update the Navigation at will using obs.frame.set_params(). That would guarantee that the changes propagate forward to anything derived from obs, such as backplanes. However, the quantities you use to define the Navigation are 2 or 3 rotation angles, not a brand new Cmatrix.

My only issue with set_cmatrix is the name, because we use "cmatrix" widely in the source code to refer to the OOPS version, whereas this function wants to return the SPICE C kernel definition, whatever it is. The solution would be to rename it get_spice_cmatrix, so there is no ambiguity. Eventually, get_spice_cmatrix will need to be implemented for all hosts, but that can come with the hosts cleanup.

[jnspitale]

I also do not like using cmatrix when we're talking about an orientation in the OOPS convention. I use "orientation matrix" to denote this in OMINAS and reserve "cmatrix" for the SPICE orientation matrix. This would be less confusing.

[rfrenchseti]

I'm assuming this should be closed now since it's been superseded by #203.