Canonicalize a coordinate only when it keeps its values - #898
Conversation
get_coord_manager re-inferred every non-CoordRange coord through get_coord to canonicalize it, but that evenness test is tolerant (rtol 1e-3), so a coord whose spacing varied by less than a tenth of a percent was replaced by an idealized ramp. The values were rewritten silently and the result claimed to be evenly sampled, so nothing downstream could tell the spacing had been invented. The collapse is still worth doing when it is a pure change of representation, so it now happens only when the resulting range reproduces the original values exactly. A CoordPartial is excluded: its values are NaN placeholders, so canonicalizing it is the whole point. This is what kept the Febus acquisition-window coord from being exact even when built with get_exact_coord, so that caveat comes off the comment and a test pins the drifting-span case end to end.
|
Warning Review limit reached
Next review available in: 34 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #898 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 182 182
Lines 21961 21971 +10
=========================================
+ Hits 21961 21971 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
A CoordRange never reaches the helper: the caller returns it before consulting it. The shape comparison could not fail either, since canonicalization re-labels a coordinate rather than resampling it, so it becomes an assert of that invariant instead of a dead branch.
1 participant
Description
Closes #896.
Passing an already-constructed
BaseCoordtoget_coord_managerdid not preserve it. Anything that was not aCoordRangewas dumped back to a dict and re-inferred throughget_coord, which snaps near-evenly-sampled values onto a regular range. Because that evenness test is tolerant —all_diffs_close_enoughusesrtol=0.001— the result was not merely a different coordinate class; the sample values themselves were silently rewritten:The same happened through the public
Patchconstructor. Afterwards the coordinate reported itself as an evenly sampledCoordRange, so nothing downstream could tell the spacing had been made up.The re-parse itself is deliberate and worth keeping: array coords can be left non-canonical by slicing, so an evenly spaced subset should collapse back to a
CoordRange. The problem is only that the collapse was allowed to move values. This keeps the canonicalization and restricts it to the case where it is a pure change of representation — if the resulting range does not reproduce the original values exactly, the coordinate that was passed in is kept.A
CoordPartialis explicitly excluded: its values are allNaNplaceholders, so it has nothing to lose and canonicalizing it is the entire point. An existing test (test_full_partial_canonicalizes_to_range) caught that when I first wrote the guard too broadly.This is the mechanism behind the caveat noted in #895, where a Febus reader could build an exact acquisition-window coordinate with
get_exact_coordand still have the coord manager convert it back to a range. That comment is now stale, so it is corrected here, and a Febus test pins the end-to-end behavior: spans that drift steadily — the shape most vulnerable to being snapped away — now survive the read.Nothing about this is Febus-specific. It applies to any coordinate carrying small, physically real irregularity: GPS-derived positions, per-sample timing jitter, measured channel spacing.
Notes
CoordRange, raw-array inference throughget_coord(data=...)is untouched, andCoordRangeinputs keep their existing short-circuit.CoordRangebuilt from a non-range, non-partial coord we were handed, so the common paths do no extra work.get_coord(data=...)should keep snapping this aggressively when inferring from a raw array is left alone. That path is documented as tolerant inference and is defensible; silently discarding a caller's explicit, exact coordinate was not.Changelog
Checklist
I have:
docs/contributing/general_guidelines.qmd).I have (if applicable):