Fittable interface #178
Fittable interface #178
Conversation
WalkthroughMajor refactor: introduces Cache and Fittable_ and propagates a refresh/freeze/version lifecycle across cadences, frames, paths, and observations; adds TimeShift/SnapCadence/FrameShift/PathShift/Platescale; standardizes ID caching and pickling; removes many "unpickled" code paths and adjusts tests and public exports. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~180 minutes Areas to focus review on:
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🧰 Additional context used🧬 Code graph analysis (1)oops/observation/observation_.py (1)
⏰ 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)
🔇 Additional comments (2)
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: 59
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
oops/path/linearpath.py (1)
41-42: Make ZERO velocity readonly for consistency.Matches readonly use elsewhere and avoids accidental mutation.
Apply this diff:
- self.vel = Vector3.ZERO + self.vel = Vector3.ZERO.as_readonly()tests/path/test_keplerpath.py (1)
47-49: Use new API: set_elements(...) instead of set_params(...).Tests currently call removed/renamed method; this will fail.
Apply this diff:
- khi.set_params(hi) - klo.set_params(lo) + khi.set_elements(hi) + klo.set_elements(lo)- khi.set_params(hi) - klo.set_params(lo) + khi.set_elements(hi) + klo.set_elements(lo)Also applies to: 98-100
oops/observation/slit1d.py (2)
155-159: Undefined attribute reference in masking logic.Line 159 references
self.midtime, but this attribute is never defined in the class. It should beself.cadence.midtime.Apply this diff to fix the undefined attribute reference:
# Apply mask to time if necessary if remask and np.any(slit_coord.mask): - time = Scalar.filled(uv.shape, self.midtime, mask=slit_coord.mask) + time = Scalar.filled(uv.shape, self.cadence.midtime, mask=slit_coord.mask)
194-195: Undefined attributeself.time.Lines 194-195 reference
self.time[0]andself.time[1], but thetimeattribute is never defined in the class. This should likely useself.cadence.timeorself.cadence.tstart/tstop.Based on the observation base class patterns, this should be:
# Time - time_min = Scalar.filled(uv_min.shape, self.time[0], mask=uv_min.mask) - time_max = Scalar.filled(uv_min.shape, self.time[1], mask=uv_min.mask) + time_min = Scalar.filled(uv_min.shape, self.cadence.tstart, mask=uv_min.mask) + time_max = Scalar.filled(uv_min.shape, self.cadence.tstop, mask=uv_min.mask)oops/frame/frame_.py (1)
274-286: Fix shortcut caching: use wayframe keys; avoid indexing FRAME_CACHE by frame_id
FRAME_CACHEis keyed by wayframe objects or (wayframe, reference_wayframe) tuples. Using a stringframe_idas a key and insertingshortcutdirectly can corrupt the cache. Convertshortcutto a wayframe and useself.wayframefor the tuple key.Apply this diff:
- if shortcut is not None: - if shortcut in Frame.FRAME_CACHE: - Frame.FRAME_CACHE[shortcut].keys -= {shortcut} - Frame.FRAME_CACHE[shortcut] = self - self.keys |= {shortcut} - - key = (Frame.FRAME_CACHE[frame_id], self.reference) - if key in Frame.FRAME_CACHE: - Frame.FRAME_CACHE[key].keys -= {key} - Frame.FRAME_CACHE[key] = self - self.keys |= {key} + if shortcut is not None: + shortcut = Frame.as_wayframe(shortcut) + if shortcut in Frame.FRAME_CACHE: + Frame.FRAME_CACHE[shortcut].keys -= {shortcut} + Frame.FRAME_CACHE[shortcut] = self + self.keys |= {shortcut} + + key = (self.wayframe, self.reference) + if key in Frame.FRAME_CACHE: + Frame.FRAME_CACHE[key].keys -= {key} + Frame.FRAME_CACHE[key] = self + self.keys |= {key}oops/frame/spicetype1frame.py (3)
118-181: Fix cache attribute naming to avoid runtime breakageLine [126] still reads
_cached_shape/self.cached_time, but the constructor only initialises_latest_*. On the first call this raisesAttributeError, and in the multi-value branches you populateself.cached_shape/self.cached_time(no underscore), so the fast path never becomes usable. The method also concludes withreturn self.latest_transform, which is another undefined attribute. Please switch the cache logic over to the_latest_*fields consistently so the method actually runs.- if np.shape(time.vals) == self._cached_shape: - diff = np.abs(time.vals - self.cached_time) - if np.all(diff < self.time_tolerance): - return self._cached_transform + if np.shape(time.vals) == self._latest_shape: + diff = np.abs(time.vals - self._latest_time) + if np.all(diff < self.time_tolerance): + return self._latest_transform @@ - self._cached_shape = time.shape - self._cached_time = cspyce.sct2e(self.spice_body_id, true_ticks) - self._cached_transform = Transform(matrix3, Vector3.ZERO, self.frame_id, - self.reference_id) - return self._cached_transform + self._latest_shape = time.shape + self._latest_time = cspyce.sct2e(self.spice_body_id, true_ticks) + self._latest_transform = Transform(matrix3, Vector3.ZERO, self.frame_id, + self.reference_id) + return self._latest_transform @@ - self.cached_shape = time.shape - self.cached_time = true_times - self.cached_transform = Transform(matrix, omega, self.frame_id, - self.reference_id) - return self.cached_transform + self._latest_shape = time.shape + self._latest_time = true_times + self._latest_transform = Transform(matrix, omega, self.frame_id, + self.reference_id) + return self._latest_transform @@ - self._latest_shape = time.shape - self._latest_time = true_times - self._latest_transform = Transform(matrix, omega, self.frame_id, - self.reference_id) - return self.latest_transform + self._latest_shape = time.shape + self._latest_time = true_times + self._latest_transform = Transform(matrix, omega, self.frame_id, + self.reference_id) + return self._latest_transform
140-180: Use actual reference frame when constructing the TransformLine [141] (and the similar block later) passes
self.reference_id, but that attribute is never defined anywhere, so we blow up withAttributeErrorthe first time the branch executes. Please pass the actual wayframe/reference objects you already have.- self._latest_transform = Transform(matrix3, Vector3.ZERO, self.frame_id, - self.reference_id) + self._latest_transform = Transform(matrix3, Vector3.ZERO, + self.wayframe, self.reference, + self.origin) @@ - self._latest_transform = Transform(matrix, omega, self.frame_id, - self.reference_id) + self._latest_transform = Transform(matrix, omega, + self.wayframe, self.reference, + self.origin)
158-159: Populate the matrix with CK output instead of itselfLine [158] currently does
matrix[...] = matrix, which just copies the array onto itself and leaves the buffer full of whatever garbagenp.emptyproduced. You want the CK resultmatrix3here so the returned transform is correct.- matrix[...] = matrix + matrix[...] = matrix3oops/path/keplerpath.py (3)
408-417: Fix derivatives for arctan2 and amplitude; signs are wrongFor angle2 = arctan2(y2, x2): d/dx = -y/(x^2+y^2); d/dy = x/(x^2+y^2). For amp2 = sqrt(x^2+y^2): d/dx = x/amp2; d/dy = y/amp2. Current signs are inverted.
Apply:
- amp2 = np.sqrt(x2**2 + y2**2) - damp2_dx2 = -x2 / amp2 - damp2_dy2 = -y2 / amp2 + amp2 = np.sqrt(x2**2 + y2**2) + damp2_dx2 = x2 / amp2 + damp2_dy2 = y2 / amp2 damp2_dt = damp2_dx2 * dx2_dt + damp2_dy2 * dy2_dt angle2 = np.arctan2(y2,x2) - dangle2_dx2 = x2 / (x2**2 + y2**2) - dangle2_dy2 = -y2 / (x2**2 + y2**2) + denom = (x2**2 + y2**2) + dangle2_dx2 = -y2 / denom + dangle2_dy2 = x2 / denom dangle2_dt = dangle2_dx2 * dx2_dt + dangle2_dy2 * dy2_dt
455-460: Fix indexing bug in wobble partialscos_arg is a scalar/array; indexing cos_arg[k] is invalid here and mismatches shapes.
Apply:
- dw_delem[...,start] = cos_arg - dw_delem[...,start+1] = self.amp[k] * cos_arg[k] - dw_delem[...,start+2] = dw_delem[...,start+1] * t + dw_delem[..., start] = cos_arg + dw_delem[..., start+1] = self.amp[k] * cos_arg + dw_delem[..., start+2] = dw_delem[..., start+1] * t
771-789: Use frame.wrt(Frame.J2000) instead of frame_wrt_j2000 (if not guaranteed)If self.frame_wrt_j2000 is not a guaranteed attribute, prefer the established wrt API.
Apply if appropriate:
- xform = self.frame_wrt_j2000.transform_at_time(time) + xform = self.frame.wrt(Frame.J2000).transform_at_time(time)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (58)
oops/__init__.py(2 hunks)oops/backplane/__init__.py(3 hunks)oops/cache.py(1 hunks)oops/cadence/__init__.py(1 hunks)oops/cadence/dualcadence.py(1 hunks)oops/cadence/metronome.py(1 hunks)oops/cadence/reshapedcadence.py(1 hunks)oops/cadence/reversedcadence.py(1 hunks)oops/cadence/snapcadence.py(1 hunks)oops/cadence/timeshift.py(1 hunks)oops/event.py(1 hunks)oops/fittable.py(1 hunks)oops/fov/__init__.py(1 hunks)oops/fov/offsetfov.py(1 hunks)oops/fov/platescale.py(1 hunks)oops/frame/__init__.py(1 hunks)oops/frame/cmatrix.py(3 hunks)oops/frame/frame_.py(6 hunks)oops/frame/frameshift.py(1 hunks)oops/frame/inclinedframe.py(2 hunks)oops/frame/laplaceframe.py(3 hunks)oops/frame/navigation.py(2 hunks)oops/frame/poleframe.py(6 hunks)oops/frame/postargframe.py(1 hunks)oops/frame/ringframe.py(3 hunks)oops/frame/rotation.py(2 hunks)oops/frame/spiceframe.py(10 hunks)oops/frame/spicetype1frame.py(8 hunks)oops/frame/spinframe.py(1 hunks)oops/frame/synchronousframe.py(2 hunks)oops/frame/trackerframe.py(2 hunks)oops/frame/twovectorframe.py(2 hunks)oops/observation/insitu.py(2 hunks)oops/observation/observation_.py(4 hunks)oops/observation/pixel.py(4 hunks)oops/observation/rasterslit1d.py(2 hunks)oops/observation/slit1d.py(5 hunks)oops/observation/snapshot.py(6 hunks)oops/observation/timedimage.py(3 hunks)oops/path/__init__.py(1 hunks)oops/path/circlepath.py(2 hunks)oops/path/coordpath.py(1 hunks)oops/path/fixedpath.py(2 hunks)oops/path/keplerpath.py(14 hunks)oops/path/linearcoordpath.py(3 hunks)oops/path/linearpath.py(2 hunks)oops/path/multipath.py(3 hunks)oops/path/path_.py(6 hunks)oops/path/pathshift.py(1 hunks)oops/path/spicepath.py(6 hunks)tests/frame/test_frame.py(1 hunks)tests/frame/test_poleframe.py(0 hunks)tests/path/test_keplerpath.py(2 hunks)tests/path/test_multipath.py(1 hunks)tests/test_cache.py(1 hunks)tests/test_fittable.py(1 hunks)tests/unittester.py(1 hunks)tests/unittester_with_hosts.py(2 hunks)
💤 Files with no reviewable changes (1)
- tests/frame/test_poleframe.py
🧰 Additional context used
🧬 Code graph analysis (53)
oops/fov/__init__.py (1)
oops/fov/platescale.py (1)
Platescale(10-99)
oops/event.py (2)
oops/backplane/__init__.py (1)
_refresh(136-202)tests/test_fittable.py (1)
_refresh(15-16)
oops/cadence/__init__.py (1)
oops/cadence/snapcadence.py (1)
SnapCadence(7-28)
oops/path/__init__.py (1)
oops/path/pathshift.py (1)
PathShift(10-88)
oops/frame/__init__.py (1)
oops/frame/frameshift.py (1)
FrameShift(10-90)
oops/cadence/reshapedcadence.py (2)
oops/cadence/reversedcadence.py (1)
_refresh(46-50)oops/fittable.py (4)
refresh(88-104)refresh(309-390)freeze(106-112)freeze(393-443)
oops/frame/frameshift.py (4)
oops/fittable.py (5)
Fittable(9-126)refresh(88-104)refresh(309-390)freeze(106-112)freeze(393-443)oops/frame/frame_.py (5)
Frame(15-660)_recover_id(458-471)_cache_id(432-456)as_primary_frame(367-376)_state_id(473-476)oops/cadence/timeshift.py (2)
_set_params(45-51)_params(54-55)oops/path/pathshift.py (2)
_set_params(44-45)_params(48-49)
oops/cadence/snapcadence.py (1)
oops/cadence/metronome.py (1)
Metronome(10-366)
oops/observation/rasterslit1d.py (1)
oops/fittable.py (3)
Fittable_(132-641)refresh(88-104)refresh(309-390)
oops/observation/insitu.py (7)
oops/fittable.py (3)
Fittable_(132-641)refresh(88-104)refresh(309-390)oops/observation/observation_.py (1)
time_shift(356-367)oops/observation/pixel.py (1)
time_shift(204-220)oops/observation/slit1d.py (1)
time_shift(238-251)oops/observation/snapshot.py (1)
time_shift(245-258)oops/observation/timedimage.py (1)
time_shift(350-361)oops/cadence/instant.py (1)
time_shift(135-143)
oops/observation/pixel.py (2)
oops/fittable.py (3)
Fittable_(132-641)refresh(88-104)refresh(309-390)oops/observation/observation_.py (2)
midtime(103-104)time(99-100)
oops/cadence/dualcadence.py (4)
oops/cadence/reshapedcadence.py (1)
_refresh(52-56)oops/cadence/reversedcadence.py (1)
_refresh(46-50)oops/cadence/timeshift.py (1)
_refresh(57-65)oops/fittable.py (4)
refresh(88-104)refresh(309-390)freeze(106-112)freeze(393-443)
tests/test_cache.py (3)
oops/cache.py (2)
Cache(9-115)clean_key(45-70)oops/frame/rotation.py (1)
Rotation(13-132)oops/path/linearpath.py (1)
LinearPath(11-95)
oops/path/pathshift.py (7)
oops/fittable.py (5)
Fittable(9-126)refresh(88-104)refresh(309-390)freeze(106-112)freeze(393-443)oops/path/path_.py (12)
Path(16-1057)_recover_id(424-437)_cache_id(398-422)as_primary_path(324-333)event_at_time(70-87)event_at_time(1105-1106)event_at_time(1163-1168)event_at_time(1217-1223)event_at_time(1273-1279)event_at_time(1313-1315)event_at_time(1352-1354)event_at_time(1433-1435)oops/cadence/timeshift.py (2)
_set_params(45-51)_params(54-55)oops/frame/frameshift.py (2)
_set_params(44-45)_params(48-49)oops/path/circlepath.py (2)
_path_key(62-63)event_at_time(81-103)oops/path/fixedpath.py (2)
_path_key(47-48)event_at_time(65-80)oops/path/spicepath.py (2)
_path_key(100-101)event_at_time(117-173)
oops/frame/frame_.py (3)
oops/cache.py (2)
Cache(9-115)clean_key(45-70)oops/fittable.py (3)
Fittable_(132-641)is_frozen(114-117)is_frozen(446-496)oops/path/path_.py (4)
id_is_temporary(389-392)_cache_id(398-422)_recover_id(424-437)_state_id(439-442)
tests/test_fittable.py (1)
oops/fittable.py (16)
Fittable(9-126)Fittable_(132-641)is_fittable(552-583)fittables(499-549)version(119-126)version(586-601)get_params(77-86)get_params(263-306)set_params(64-75)set_params(186-260)freeze(106-112)freeze(393-443)refresh(88-104)refresh(309-390)is_frozen(114-117)is_frozen(446-496)
oops/cadence/timeshift.py (5)
oops/fittable.py (7)
Fittable(9-126)set_params(64-75)set_params(186-260)refresh(88-104)refresh(309-390)freeze(106-112)freeze(393-443)oops/cadence/reshapedcadence.py (3)
_refresh(52-56)time_shift(427-435)as_continuous(438-445)oops/frame/frameshift.py (2)
_set_params(44-45)_params(48-49)oops/path/pathshift.py (2)
_set_params(44-45)_params(48-49)oops/cadence/metronome.py (2)
time_shift(305-314)as_continuous(317-324)
oops/frame/cmatrix.py (2)
oops/fittable.py (5)
Fittable_(132-641)refresh(88-104)refresh(309-390)freeze(106-112)freeze(393-443)oops/frame/frame_.py (4)
Frame(15-660)_recover_id(458-471)_cache_id(432-456)_state_id(473-476)
oops/frame/inclinedframe.py (4)
oops/fittable.py (5)
Fittable_(132-641)freeze(106-112)freeze(393-443)refresh(88-104)refresh(309-390)oops/frame/frame_.py (8)
Frame(15-660)_recover_id(458-471)_cache_id(432-456)as_primary_frame(367-376)_state_id(473-476)transform_at_time(81-103)transform_at_time(708-710)transform_at_time(748-749)oops/frame/rotation.py (3)
Rotation(13-132)_refresh(75-91)transform_at_time(111-132)oops/frame/spinframe.py (2)
_frame_key(69-70)transform_at_time(87-116)
oops/frame/twovectorframe.py (2)
oops/fittable.py (5)
Fittable_(132-641)refresh(88-104)refresh(309-390)freeze(106-112)freeze(393-443)oops/frame/frame_.py (9)
Frame(15-660)as_wayframe(380-388)register(248-351)register(712-713)register(745-746)register(1015-1016)_cache_id(432-456)as_primary_frame(367-376)_state_id(473-476)
oops/path/linearpath.py (5)
oops/event.py (8)
Event(13-2103)frame(293-294)pos(273-275)origin(285-286)vel(278-282)shape(305-315)state(268-270)time(264-265)oops/fittable.py (5)
Fittable_(132-641)refresh(88-104)refresh(309-390)freeze(106-112)freeze(393-443)oops/path/path_.py (13)
Path(16-1057)as_waypoint(337-346)_recover_id(424-437)_cache_id(398-422)_state_id(439-442)event_at_time(70-87)event_at_time(1105-1106)event_at_time(1163-1168)event_at_time(1217-1223)event_at_time(1273-1279)event_at_time(1313-1315)event_at_time(1352-1354)event_at_time(1433-1435)oops/path/circlepath.py (2)
_path_key(62-63)event_at_time(81-103)oops/path/fixedpath.py (2)
_path_key(47-48)event_at_time(65-80)
tests/path/test_keplerpath.py (1)
oops/path/keplerpath.py (2)
KeplerPath(33-826)get_elements(233-236)
oops/frame/poleframe.py (5)
oops/cache.py (1)
Cache(9-115)oops/fittable.py (5)
Fittable_(132-641)refresh(88-104)refresh(309-390)freeze(106-112)freeze(393-443)oops/frame/frame_.py (7)
Frame(15-660)_recover_id(458-471)_cache_id(432-456)_state_id(473-476)transform_at_time(81-103)transform_at_time(708-710)transform_at_time(748-749)oops/frame/laplaceframe.py (3)
_refresh(76-77)_frame_key(83-84)transform_at_time(101-198)oops/frame/ringframe.py (3)
_refresh(78-84)_frame_key(90-91)transform_at_time(109-171)
oops/frame/spicetype1frame.py (4)
oops/path/path_.py (6)
frame_id(98-99)register(191-307)register(1109-1110)register(1160-1161)register(1442-1443)_state_id(439-442)oops/spice_support.py (1)
body_id_and_name(42-76)oops/frame/frame_.py (7)
Frame(15-660)as_wayframe(380-388)register(248-351)register(712-713)register(745-746)register(1015-1016)_state_id(473-476)oops/transform.py (2)
shape(100-112)Transform(7-366)
oops/fov/platescale.py (3)
oops/fittable.py (1)
Fittable(9-126)oops/fov/fov_.py (1)
FOV(10-818)oops/fov/offsetfov.py (3)
_set_params(68-72)xy_from_uvt(88-110)uv_from_xyt(113-134)
oops/path/path_.py (6)
oops/cache.py (2)
Cache(9-115)clean_key(45-70)oops/fittable.py (3)
Fittable_(132-641)is_frozen(114-117)is_frozen(446-496)oops/frame/frame_.py (8)
register(248-351)register(712-713)register(745-746)register(1015-1016)id_is_temporary(417-420)_cache_id(432-456)_recover_id(458-471)_state_id(473-476)oops/path/fixedpath.py (1)
_path_key(47-48)oops/path/keplerpath.py (1)
_path_key(242-243)oops/path/multipath.py (1)
_path_key(65-66)
oops/backplane/__init__.py (2)
oops/event.py (4)
time(264-265)shape(305-315)wod(374-379)with_los_derivs(1098-1110)oops/observation/observation_.py (6)
time(99-100)timegrid(509-608)meshgrid(465-506)midtime(103-104)event_at_grid(611-634)gridless_event(637-664)
oops/path/linearcoordpath.py (2)
oops/fittable.py (5)
Fittable_(132-641)refresh(88-104)refresh(309-390)freeze(106-112)freeze(393-443)oops/path/path_.py (4)
Path(16-1057)_recover_id(424-437)_cache_id(398-422)_state_id(439-442)
oops/path/fixedpath.py (3)
oops/event.py (7)
Event(13-2103)frame(293-294)pos(273-275)origin(285-286)shape(305-315)state(268-270)time(264-265)oops/fittable.py (5)
Fittable_(132-641)refresh(88-104)refresh(309-390)freeze(106-112)freeze(393-443)oops/path/path_.py (5)
Path(16-1057)as_waypoint(337-346)_recover_id(424-437)_cache_id(398-422)_state_id(439-442)
oops/frame/ringframe.py (3)
oops/cache.py (1)
Cache(9-115)oops/fittable.py (5)
Fittable_(132-641)refresh(88-104)refresh(309-390)freeze(106-112)freeze(393-443)oops/frame/frame_.py (4)
Frame(15-660)_recover_id(458-471)_cache_id(432-456)_state_id(473-476)
oops/__init__.py (3)
oops/cache.py (1)
Cache(9-115)oops/frame/frame_.py (1)
Frame(15-660)oops/path/path_.py (1)
Path(16-1057)
oops/path/coordpath.py (3)
oops/event.py (7)
Event(13-2103)pos(273-275)origin(285-286)frame(293-294)shape(305-315)state(268-270)time(264-265)oops/fittable.py (5)
Fittable_(132-641)refresh(88-104)refresh(309-390)freeze(106-112)freeze(393-443)oops/path/path_.py (4)
Path(16-1057)_recover_id(424-437)_cache_id(398-422)_state_id(439-442)
tests/frame/test_frame.py (1)
oops/frame/rotation.py (1)
Rotation(13-132)
oops/frame/laplaceframe.py (3)
oops/cache.py (1)
Cache(9-115)oops/fittable.py (5)
Fittable_(132-641)refresh(88-104)refresh(309-390)freeze(106-112)freeze(393-443)oops/frame/frame_.py (4)
Frame(15-660)_recover_id(458-471)_cache_id(432-456)_state_id(473-476)
oops/frame/synchronousframe.py (4)
oops/fittable.py (4)
refresh(88-104)refresh(309-390)freeze(106-112)freeze(393-443)oops/frame/frame_.py (5)
Frame(15-660)as_wayframe(380-388)_recover_id(458-471)_cache_id(432-456)_state_id(473-476)oops/transform.py (1)
Transform(7-366)oops/path/path_.py (12)
frame_id(98-99)_recover_id(424-437)_cache_id(398-422)_state_id(439-442)event_at_time(70-87)event_at_time(1105-1106)event_at_time(1163-1168)event_at_time(1217-1223)event_at_time(1273-1279)event_at_time(1313-1315)event_at_time(1352-1354)event_at_time(1433-1435)
oops/frame/navigation.py (5)
oops/fittable.py (5)
Fittable(9-126)refresh(88-104)refresh(309-390)set_params(64-75)set_params(186-260)oops/frame/frame_.py (8)
Frame(15-660)as_wayframe(380-388)_recover_id(458-471)as_primary_frame(367-376)_state_id(473-476)transform_at_time(81-103)transform_at_time(708-710)transform_at_time(748-749)oops/transform.py (2)
Transform(7-366)shape(100-112)oops/frame/cmatrix.py (2)
_refresh(48-50)transform_at_time(131-152)oops/frame/rotation.py (4)
_refresh(75-91)_set_params(59-69)_params(72-73)transform_at_time(111-132)
oops/observation/slit1d.py (4)
oops/observation/observation_.py (3)
time(99-100)midtime(103-104)time_shift(356-367)oops/cadence/snapcadence.py (1)
SnapCadence(7-28)oops/fittable.py (3)
Fittable_(132-641)refresh(88-104)refresh(309-390)oops/observation/snapshot.py (1)
time_shift(245-258)
oops/cache.py (2)
oops/path/path_.py (1)
as_waypoint(337-346)oops/frame/frame_.py (1)
as_wayframe(380-388)
oops/observation/observation_.py (2)
oops/event.py (3)
frame(293-294)time(264-265)copy(901-962)oops/frame/navigation.py (1)
Navigation(13-152)
oops/observation/timedimage.py (1)
oops/fittable.py (3)
Fittable_(132-641)refresh(88-104)refresh(309-390)
oops/frame/trackerframe.py (3)
oops/cache.py (1)
Cache(9-115)oops/fittable.py (5)
Fittable_(132-641)refresh(88-104)refresh(309-390)freeze(106-112)freeze(393-443)oops/frame/frame_.py (6)
Frame(15-660)as_frame(355-363)as_wayframe(380-388)_recover_id(458-471)_cache_id(432-456)_state_id(473-476)
oops/cadence/reversedcadence.py (4)
oops/cadence/dualcadence.py (1)
_refresh(49-54)oops/cadence/reshapedcadence.py (1)
_refresh(52-56)oops/cadence/timeshift.py (1)
_refresh(57-65)oops/observation/observation_.py (2)
time(99-100)midtime(103-104)
oops/fittable.py (7)
oops/backplane/__init__.py (1)
_refresh(136-202)oops/cadence/timeshift.py (3)
_refresh(57-65)_set_params(45-51)_params(54-55)oops/fov/platescale.py (3)
_refresh(38-39)_set_params(41-42)_params(45-46)oops/frame/rotation.py (3)
_refresh(75-91)_set_params(59-69)_params(72-73)oops/path/keplerpath.py (2)
_set_params(166-168)_params(171-172)oops/path/pathshift.py (2)
_set_params(44-45)_params(48-49)tests/test_fittable.py (4)
_set_params(18-19)_set_params(42-43)_params(22-23)_params(46-47)
oops/frame/spinframe.py (2)
oops/fittable.py (5)
Fittable_(132-641)refresh(88-104)refresh(309-390)freeze(106-112)freeze(393-443)oops/frame/frame_.py (8)
Frame(15-660)_recover_id(458-471)register(248-351)register(712-713)register(745-746)register(1015-1016)_cache_id(432-456)_state_id(473-476)
oops/fov/offsetfov.py (9)
oops/cadence/timeshift.py (1)
_set_params(45-51)oops/fov/platescale.py (1)
_set_params(41-42)oops/frame/frameshift.py (1)
_set_params(44-45)oops/frame/navigation.py (1)
_set_params(79-86)oops/frame/rotation.py (1)
_set_params(59-69)oops/path/keplerpath.py (1)
_set_params(166-168)oops/path/pathshift.py (1)
_set_params(44-45)tests/test_fittable.py (2)
_set_params(18-19)_set_params(42-43)oops/fov/fov_.py (1)
xy_from_uv(118-141)
oops/path/circlepath.py (4)
oops/cache.py (1)
Cache(9-115)oops/fittable.py (5)
Fittable_(132-641)refresh(88-104)refresh(309-390)freeze(106-112)freeze(393-443)oops/path/path_.py (5)
Path(16-1057)as_waypoint(337-346)_recover_id(424-437)_cache_id(398-422)_state_id(439-442)oops/path/coordpath.py (1)
_path_key(49-50)
oops/path/spicepath.py (1)
oops/path/path_.py (3)
Path(16-1057)_cache_id(398-422)_state_id(439-442)
oops/frame/spiceframe.py (1)
oops/frame/frame_.py (16)
Frame(15-660)register(248-351)register(712-713)register(745-746)register(1015-1016)_state_id(473-476)transform_at_time(81-103)transform_at_time(708-710)transform_at_time(748-749)quick_frame(540-660)transform_at_time_if_possible(106-134)transform_at_time_if_possible(751-752)transform_at_time_if_possible(811-819)transform_at_time_if_possible(869-877)transform_at_time_if_possible(908-910)transform_at_time_if_possible(1011-1013)
oops/path/keplerpath.py (3)
oops/cache.py (1)
Cache(9-115)oops/path/path_.py (5)
Path(16-1057)_recover_id(424-437)_cache_id(398-422)_state_id(439-442)photon_to_event(531-539)oops/fittable.py (5)
Fittable(9-126)refresh(88-104)refresh(309-390)freeze(106-112)freeze(393-443)
oops/frame/postargframe.py (3)
oops/fittable.py (3)
Fittable_(132-641)freeze(106-112)freeze(393-443)oops/frame/frame_.py (12)
Frame(15-660)as_wayframe(380-388)_recover_id(458-471)register(248-351)register(712-713)register(745-746)register(1015-1016)_cache_id(432-456)_state_id(473-476)transform_at_time(81-103)transform_at_time(708-710)transform_at_time(748-749)oops/frame/cmatrix.py (2)
_frame_key(56-57)transform_at_time(131-152)
oops/observation/snapshot.py (5)
oops/observation/observation_.py (4)
insert_subfield(399-403)time(99-100)midtime(103-104)time_shift(356-367)oops/cadence/snapcadence.py (1)
SnapCadence(7-28)oops/fittable.py (3)
Fittable_(132-641)refresh(88-104)refresh(309-390)oops/cadence/metronome.py (1)
time_shift(305-314)oops/observation/pixel.py (1)
time_shift(204-220)
oops/frame/rotation.py (6)
oops/frame/frame_.py (11)
Frame(15-660)as_wayframe(380-388)register(248-351)register(712-713)register(745-746)register(1015-1016)as_primary_frame(367-376)_state_id(473-476)transform_at_time(81-103)transform_at_time(708-710)transform_at_time(748-749)oops/fittable.py (5)
Fittable(9-126)set_params(64-75)set_params(186-260)refresh(88-104)refresh(309-390)oops/transform.py (2)
shape(100-112)Transform(7-366)oops/cadence/timeshift.py (3)
_refresh(57-65)_set_params(45-51)_params(54-55)oops/frame/navigation.py (4)
_refresh(92-106)_set_params(79-86)_params(89-90)transform_at_time(131-152)oops/frame/spinframe.py (1)
transform_at_time(87-116)
oops/path/multipath.py (3)
oops/fittable.py (5)
Fittable_(132-641)refresh(88-104)refresh(309-390)freeze(106-112)freeze(393-443)oops/path/path_.py (4)
Path(16-1057)_recover_id(424-437)_cache_id(398-422)_state_id(439-442)oops/path/spicepath.py (1)
_path_key(100-101)
⏰ 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-macos, 3.13)
- GitHub Check: Test oops (self-hosted-windows, 3.10)
- GitHub Check: Test oops (self-hosted-linux, 3.13)
- GitHub Check: Test oops (self-hosted-macos, 3.11)
- 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.12)
- GitHub Check: Test oops (self-hosted-linux, 3.10)
🔇 Additional comments (43)
oops/event.py (1)
395-405: Method rename aligns with Fittable interface patternThe rename from
reinitto_refreshfollows the established pattern across the codebase for internal state management methods. This maintains consistency with the broader Fittable infrastructure being introduced.oops/backplane/__init__.py (3)
134-135: Good refactoring: Initialization consolidated in_refreshMoving the initialization logic into
_refreshfollows the established pattern and ensures consistency with the Fittable interface requirements.Based on learnings
136-201: Well-structured_refreshimplementationThe
_refreshmethod properly:
- Handles time computation with fallback to observation timegrid
- Collapses uniform times to simplify computation
- Initializes all required internal state dictionaries
- Follows the expected pattern for Fittable objects
This implementation aligns with the broader refactoring pattern seen across the codebase.
158-159: No change needed:gridless_eventis already invoked correctly The callself.obs.gridless_event(self.meshgrid, time=self.time)includes parentheses and matches the method definition.Likely an incorrect or invalid review comment.
oops/cadence/metronome.py (1)
53-58: Pickle round‑trip tuple matches init — LGTMState tuple aligns with constructor args; setstate reuses init correctly.
oops/cadence/__init__.py (1)
12-12: Expose SnapCadence in public API — LGTMImport wiring is correct and consistent with new class location.
oops/frame/__init__.py (1)
9-9: Export FrameShift — LGTMKeeps the frame namespace consistent with newly added class.
oops/observation/timedimage.py (1)
28-61: Constructor docstring/name correction — LGTMAccurately documents TimedImage.
oops/__init__.py (2)
48-51: Top‑level exports for Cache and Fittable_ — LGTMPublic surface expansion is clear and consistent with new modules.
86-88: No reordering required: package import semantics ensureCache.PATH_CLASSandCache.FRAME_CLASSare set in__init__.pybefore anyclean_keyinvocation.oops/path/circlepath.py (2)
48-56: LGTM! Proper integration with the new Fittable API and Path registry.The initialization correctly:
- Recovers existing path IDs when appropriate
- Registers the path properly
- Caches the path ID for future reuse
65-75: LGTM! Well-implemented serialization with proper state management.The serialization methods correctly handle the new Fittable state management pattern by refreshing before pickling and freezing after unpickling.
oops/path/pathshift.py (1)
44-49: LGTM! Proper Fittable interface implementation.The Fittable interface is correctly implemented with
_set_paramsand_paramsproperty following the expected pattern.oops/frame/spiceframe.py (4)
19-20: LGTM! Proper use of keyword-only parameters for optional configuration.The updated constructor signature properly uses keyword-only parameters for
omega_typeandomega_dt, following Python best practices for API design.
86-87: LGTM! Consistent registration pattern.The change from conditional registration to always registering aligns with the broader refactoring pattern across the codebase.
93-102: LGTM! Well-implemented serialization support.The serialization methods properly handle state management with the new signature and correctly recover frame IDs from the translation dictionary when available.
175-175: Verify numerical precision implications of omega_dt minimum.Line 175 takes the minimum of two time steps when
omega_numericalis true. Ensure this doesn't cause numerical instability for derivatives ifself.omega_dtis significantly smaller than the configured step.The choice of minimum time step could affect the quality of numerical derivatives. Consider documenting this behavior or adding validation to ensure the time step remains within reasonable bounds for numerical stability.
tests/test_fittable.py (3)
10-24: LGTM! Clean implementation of the test harness.The helper classes A, B, C, and D are well-structured for testing various Fittable scenarios. Class A correctly implements the required Fittable interface with
_refresh,_set_params, and_paramsproperty.
119-120: Good test coverage for dict-based parameter setting.The test correctly validates the dict-based
set_paramswith both''(for the object itself) and named sub-object keys.
35-52: No infinite recursion: Fittable.refresh memoizes circular refs
Fittable_.refresh’s internal_memoset (lines 330–336) skips already-visited sub-objects, soself.c = selfand its.cchain won’t recurse infinitely. No changes needed.tests/test_cache.py (3)
76-78: LGTM! Proper validation of path waypoint conversion.The test correctly verifies that LinearPath objects are converted to hashable waypoints via
Cache.clean_key.
84-87: Comprehensive test for composite key cleaning.Good coverage of mixed types in tuple keys, ensuring all components are properly converted to hashable forms.
118-124: Good edge case testing for maxsize=0.The test properly validates that a cache with maxsize=0 doesn't store any items and returns None for all lookups.
oops/observation/slit1d.py (3)
112-114: Appropriate use of Fittable_.refresh before serialization.The addition of
Fittable_.refresh(self)before state export ensures internal consistency before pickling.
248-251: LGTM! Proper cadence-based time shifting.The
time_shiftmethod correctly delegates to the cadence's time shift method and reconstructs the Slit1D with the shifted cadence.
97-105: Inconsistency in texp handling when Cadence is provided.When a Cadence object is passed as
tstart, the code setsself.texp = self.cadence.texp(line 101). However, the constructor'stexpparameter is then ignored but still stored asself.texp = texpon line 104. This creates an inconsistency whereself.texpmight not matchself.cadence.texp.Apply this diff to fix the inconsistency:
if isinstance(tstart, Cadence): self.cadence = tstart if self.cadence.shape != (1,): raise ValueError("Shape of a Snapshot's cadence must be (1,)") self.texp = self.cadence.texp else: self.cadence = SnapCadence(tstart, texp) self.texp = texpshould be:
if isinstance(tstart, Cadence): self.cadence = tstart if self.cadence.shape != (1,): raise ValueError("Shape of a Snapshot's cadence must be (1,)") self.texp = self.cadence.texp else: + self.texp = texp self.cadence = SnapCadence(tstart, texp) - self.texp = texpActually, the current code is correct but could be clearer. Consider removing line 104 entirely since
self.texpis already set appropriately in both branches.Likely an incorrect or invalid review comment.
oops/path/fixedpath.py (1)
51-55: Serialization flow looks correctRefreshing, caching the ID, and saving the primary origin/frame with
_state_id()aligns with the new Fittable/identity pattern.oops/frame/twovectorframe.py (1)
66-68: Degenerate node when z-axes align (potential NaNs).
Vector3.ZAXIS.ucross(z_axis)is undefined ifz_axis≈ ±Z; consider masking or a fallback direction to avoid NaNs.As a follow-up, confirm whether other frame classes handle this degeneracy similarly and whether masking is the expected behavior.
oops/frame/poleframe.py (4)
26-27: Good API improvement with keyword-only parameters.Making
retrograde,aries,frame_id, andcache_sizekeyword-only parameters improves API clarity and prevents positional argument confusion.
73-74: Cache initialization pattern is consistent.The new cache management pattern using
_refresh()to initialize theCacheinstance is well-structured and follows the pattern established in other frame classes.Also applies to: 91-92, 94-95
105-108: Serialization properly manages state refresh.Good practice calling
Fittable_.refresh(self)before serialization to ensure all cached information is current before state capture.
141-142: Cache access pattern looks correct.The caching logic properly checks for cached values and stores new computations. The use of
Cache.clean_keyis handled internally by the Cache class.Also applies to: 180-181
oops/frame/laplaceframe.py (4)
30-30: Good API consistency with keyword-only parameters.Making
frame_idandcache_sizekeyword-only aligns with the pattern in PoleFrame and improves API clarity.
46-52: Internal state management looks well-structured.The initialization of private attributes (
_planet,_orbit_frame,_planet_frame,_cos_tilt,_sin_tilt) provides clear encapsulation and pre-computation of frequently used values.
28-28: Require backward-compatibility check for FRAME_IDS rename Renaming the public constantFRAME_IDSto_FRAME_IDSchanges the external API; ensure no consumers rely onLaplaceFrame.FRAME_IDSor provide a compatibility alias/deprecation.
65-67: orbit.planet.ring_frame access is safe. KeplerPath.init always sets planet via Path.BODY_CLASS.as_body(), returning a registered Body with ring_frame initialized.Likely an incorrect or invalid review comment.
oops/path/multipath.py (5)
54-59: Good addition of indexing support.The
__getitem__method provides intuitive array-like access to individual paths or sub-MultiPaths, with proper origin and frame propagation.
69-72: Serialization follows the new pattern correctly.The serialization properly uses
Fittable_.refresh()before state capture andFittable_.freeze()after state restoration, following the new unified pattern.
43-48: The onlyMultiPath(..., path_id='+')use is in the updated test which expects the joined IDs ("SUN+EARTH+MOON"). No other call sites rely on the old append-only behavior. This change is intentional and covered by tests.
108-111: Remove manual bounds check:Qube.broadcastenforces thattime.values’s trailing dimensions matchself.shape(or raises on mismatch), so indexing withindexis safe.Likely an incorrect or invalid review comment.
65-66: No changes needed: clean_key converts the list from _path_key into an immutable tuple
Cache.clean_keydetects list (and tuple) keys and wraps each cleaned item in a tuple, so the list returned by_path_keyis correctly converted to an immutable cache key.oops/path/keplerpath.py (2)
717-745: No issues: Cache.getitem returns None on miss.
245-255: ID caching on pickle for deterministic path IDsFor consistency with Frame/Path patterns, cache the temporary ID during serialization after refresh/freeze checks.
Apply:
def __getstate__(self): - self.refresh() - self._cache_id() + self.refresh() + self._cache_id() return (self.planet, self.epoch, self.elements, Path.as_primary_path(self.observer), self.wobbles, self._state_id())(No behavior change; this ensures ID is recorded when frozen.) As per coding guidelines.
Likely an incorrect or invalid review comment.
|
Please remove all references to Python 3.9 from |
This pull request completes my thinking about the
Fittableinterface.fittable.pyhas receive a major rewrite. The classFittablenow handles most of the infrastructure for managing "fittable" (mutable) objects, leaving a much smaller set things that need to be implemented on a class-by-class basis. It also supports situations where a class is not intrinsically fittable, but one of its sub-object might be.Fittablesub-objects requires a small amount of refactoring. Basically, any object that might have a fittable sub-object, and which caches internal information on the assumption that its sub-objects are static, now needs a new function_refresh, which will update internal information based on its subobjects. This has now been implemented forBackplanesand for many other classes.objthat might have one of its sub-objects modified underneath, callFittseable_.refresh(obj)first. This is a quick operation unless the object or a sub-object has changed, it which case it ensures that_refreshgets called and everything is in sync internally.Path,Frame, andObservationsubclasses have all been updated:Fittablesupport, even for object that are not intrinsicallyFittable.PathsandFrameswhen an identicalPathorFramealready exists. If a new object has the same definition as an existing object, it will be assigned the same ID by default. This is particularly useful for objects being read from pickle files. New methods_path_keyand_frame_keysupport this mechanism.These new "placeholder" classes have been written but not tested. They take advantage of the updated
FittableAPI. I will add an issue to test them fully but I wanted to capture my thinking of how they should work while it's fresh in my mind.TimeShift: AFittabletime-shift applied to an observation'sCadence.PathShift: AFittabletime-shift applied to aPath.FrameShift: AFittabletime-shift applied toFrame.Platescale: AFittablescale factor applied to an FOV.New class
SnapCadencesubclassesMetronomefor situations in which a Cadence contains a single time-step (as is the case forSnapshotObservations).New class
Cachereplaces several ad-hoc implementations of caching inside other classes. This emulatesfunctools.lru_cachebut can be used in places where arguments to a function are mutable and/or if it is not practical to wrap an entire function.Cache.clean_keyconverts a mutable key into an immutable one, making it suitable for indexing a dict.The practical impact of these changes should be minimal at this time. Unit tests pass (although a few required minor modifications). Coverage should be about the same as before. There are new unit tests for the
CacheandFittableclasses.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.