Store coordinate envelopes in original units; a bare range means native units - #872
Conversation
|
Warning Review limit reached
Next review available in: 13 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 (7)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change preserves original coordinate units in index envelopes and dataframe results. Bare ranges use native units, while quantity ranges convert during query or assembly. Chunk planning normalizes compatible units by dimensionality. The index schema version changes to 8. ChangesNative coordinate units
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@dascore/io/index/planned.py`:
- Around line 402-411: Update the unit comparison in the surrounding trim-hint
logic to treat null or NaN plan units as missing, matching the existing
_in_plan_units behavior. Use the established pandas null check before comparing
plan_units and source_units, while preserving hint removal when both units are
present but differ.
In `@dascore/utils/chunk_plan.py`:
- Around line 286-311: Update _normalize_chunk_units around the envelope
conversions using to_numpy(dtype=float) so datetime64 and timedelta64 values are
not passed to numeric unit conversion. Restrict this normalization path to
numeric envelopes, or convert time-like rows through their stored nanosecond
fields while preserving canonical "s" units and the existing mixed-unit
normalization behavior.
🪄 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: CHILL
Plan: Pro Plus
Run ID: ebbb55d6-caa7-4e70-854d-3fed45e5f4eb
📒 Files selected for processing (23)
dascore/core/spool.pydascore/io/index/backend.pydascore/io/index/catalog.pydascore/io/index/indexer.pydascore/io/index/ingest.pydascore/io/index/planned.pydascore/io/index/query.pydascore/io/index/schema.pydascore/utils/chunk_plan.pydascore/utils/misc.pydascore/utils/patch_assembly.pydascore/utils/pd.pydocs/changelog.qmddocs/notes/spool_chunking.qmddocs/notes/spool_index.qmddocs/notes/spool_selection.qmdtests/test_core/test_patch_chunk.pytests/test_core/test_spool_select_spec.pytests/test_io/test_index/test_catalog.pytests/test_io/test_index/test_heterogeneity_stress.pytests/test_io/test_index/test_index_contract.pytests/test_io/test_index/test_index_edge_cases.pytests/test_utils/test_chunk.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #872 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 177 177
Lines 20017 20153 +136
==========================================
+ Hits 20017 20153 +136
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:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78ad481023
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| min_num=float(summary.min), | ||
| max_num=float(summary.max), | ||
| step_num=None if pd.isnull(step) else float(step), |
There was a problem hiding this comment.
When a view contains compatible coordinates expressed in different units, storing these raw native magnitudes makes PatchCatalog._relative_to_absolute() compute its global minimum and maximum from incomparable values. For example, identical spans represented as 0–300 m and 0–984 ft produce a global maximum of 984, after which the resulting bare bounds are interpreted natively by both patches; relative=True therefore trims different physical intervals and can leave one patch effectively untrimmed. Convert the envelopes to a common unit, or resolve the offsets per unit group, before calculating the global range.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and left as-is for the same reason as the sort comment — with a note added, since this one is a global computation rather than per-file. Verified: across 0-299 m and 0-981 ft the global envelope becomes 0-981, so a relative offset trims different physical intervals and can leave the metre patch effectively untrimmed. That is the mixed-spelling archive again: 'ten percent of the spool's span' has no single meaning when the members do not share a unit, exactly as a bare bound does not. Converting for this one path would mean picking a canonical unit again, which is what the PR removes. Worth revisiting if mixed-unit archives turn out to be common in practice — for a unit-homogeneous archive, the normal case, relative offsets are unchanged.
| if typed.units is None: | ||
| val = bound | ||
| else: | ||
| val = convert_units( | ||
| bound, to_units=unit, from_units=typed.units | ||
| ) |
There was a problem hiding this comment.
For a range containing one bare bound and one quantity bound, _range_bounds() compares their unconverted magnitudes before this per-unit branch runs. Thus a feet coordinate queried with distance=(100, 50 * m) raises lo > hi because it compares 100 to 50, even though the converted interval is valid (100–164 ft). Mixed ranges are explicitly handled here, so their ordering must be validated only after each bound has been expressed in the stored coordinate unit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f0ab669. Confirmed the divergence: spool.select(distance=(100, 50 * m)) on a feet coordinate raised lo > hi while patch.select returned 101.71-164.04 ft — exactly the class of Patch/Spool mismatch this PR exists to remove. The ordering check now runs only when both bounds share a frame of reference; a mixed range is validated per stored unit in build_coord_clause, where each bound is expressed against the definition being tested. Genuinely reversed ranges, bare or quantity, still raise.
| min_num=float(summary.min), | ||
| max_num=float(summary.max), |
There was a problem hiding this comment.
When patches express the sorted coordinate in compatible but different units, Spool.sort() still orders directly by the stored numeric minimum in _order_clause, but these values are now native magnitudes. For example, a patch starting at 2 m is placed before one starting at 3 ft because SQL compares 2 with 3, although 3 ft is physically smaller; the previous canonical storage ordered this correctly. Coordinate sort keys need conversion to a common unit before comparison.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed but intended, so not changed. Ordering a mixed-unit archive by native magnitudes is the accepted consequence of a bare number meaning the coordinate's own units — the same rule that makes a bare range select a per-file interval there. A spool spanning several spellings has no single unit to sort in, and reintroducing a canonical one for the sort key alone would restore precisely the patch/spool divergence this PR removes. It is documented in the changelog and in the selection note; pass a quantity when one physical ordering is what you want. Unit-homogeneous archives, the normal case, are unaffected.
|
✅ Documentation built: |
… units The index recorded numeric coordinate envelopes converted to base SI beside a units column holding the base unit, so a degrees coordinate showed radians and a bare spool selector meant SI while the same range on the patch meant the coordinate's own units (#863). Envelopes now store the original magnitudes beside the original unit string — the index shows what the patch shows — and a bare numeric range means native units everywhere: select, chunk lengths, and the residual trim, which passes bare ranges straight through to Patch.select. Quantity queries convert themselves once per distinct compatible stored unit into OR branches inside the existing coord_defs semi-join, and the chunk planner re-spells compatible unit families to one unit per dimensionality before partitioning, so metres still merge with feet. Assembly hands trims down as quantities in the plan's unit. The index version is bumped; a quantity chunk length on a frame recording no units now raises instead of silently assuming SI.
get_contents (and the indexer's flat relation) now expose each private
_{name}_units column under its public name, so a native envelope value
sits beside the unit that scales it; the private spelling remains for
the planners, whose merge policing compares public columns. The
presented-envelope adjustment converts a quantity range per distinct
row unit instead of applying SI magnitudes to native columns. The
bare_is_si machinery is gone: bare ranges pass through everywhere, and
only unit-bearing queries canonicalize.
Codex and two adversarial agents, five confirmed defects: - A bare bound beside a quantity bound was reinterpreted in the quantity's unit at load; _CanonicalRange now carries units per bound, so bare bounds stay native exactly as Patch.select reads them. - Re-planning a quantity-selected derived view applied SI magnitudes to native envelopes; collapse_working_df now converts per row unit through the shared magnitudes_in kernel. - The coord-def dedup key was a physically-simplified fingerprint, so one coordinate spelled in metres and feet collided and the first spelling's metadata lied for the second (false select negatives, mislabeled unions). The stored key now carries the unit spelling; fingerprints stay semantic. - Plan trims are magnitudes in the partition's normalized unit, but members from differently-spelled sources read them natively (empty patches, wrong intervals). Members now carry the plan unit, trims convert as quantities at assembly, and a bare read hint is dropped when the source spells the coordinate differently. - Affine quantity chunk lengths converted absolutely — 20 degC of extent became 68 degF; lengths now convert as deltas, which also accepts scaled unit spellings pint's .to() rejects. An attr named like a coordinate's units column is omitted from the flat view with a warning (coord wins, same rule as envelope collisions), the query layer treats "" units as unset, and the remaining SI-era comments and tests now state the native-units contract.
The Codex verification round found three defects this branch had
introduced:
- A chunk output drawing on a single member never converted to the
partition's unit, because only merging visits that conversion. A feet
member was therefore published under a row claiming metres, so
get_contents described an envelope no patch it yields actually had and
a bare select on that envelope matched nothing. Members are now
re-expressed in the plan's unit as they load (identity mode, which
promises the untouched patch, is exempt), which also makes the
merge-time normalizer a no-op rather than the only guard.
- The column recording a member's own unit spelling could be a real
coordinate's: a coordinate named "{dim}_source" owns
"_{dim}_source_units" outright. It is now "_{dim}_units_source", which
no coordinate unit column can spell.
- Re-chunking a derived view renamed that column a second time, leaving
two columns of one name; one silently vanished when the rows became
load kwargs, so a member could be trimmed in the wrong unit.
The units presentation helper documents why it never overwrites a public
column, and the merge-time normalizer's conversion branch is now tested
directly since assembly normalizes before it runs.
get_quantity is nullable, so the type checker rejected reading to_base_units off it. Null and empty spellings are filtered out a few lines above, so the assert states what the filter already guarantees.
Checked what the released version actually does: its index describes only time, so a coordinate range on any other dimension was never evaluated by the index at all — it fell through to the patch at load, which reads it in the coordinate's own units, and get_contents showed no distance columns to misreport. Bare ranges have therefore always meant native units; the canonical-SI storage arrived with the unreleased index rewrite and never shipped. So this entry no longer claims to change what a bare range means. It describes what a reader upgrading from the last release actually gets: coordinates other than time indexed at all, their envelopes and units reported truthfully, and quantities as the way to mean one physical interval. The entry describing a chunk fix for the same unreleased regression is dropped — its net effect for users is zero and its closing clause is no longer true — and the raise-without-units and delta-length rules move to the bullet for the quantity chunk feature they belong to.
- A range mixing a bare bound with a unit-bearing one held magnitudes in two frames of reference, and the ordering check compared them raw: distance=(100, 50 * m) on a feet coordinate was rejected as lo > hi though the patch accepts it as 100-164 ft. The check now runs only when both bounds share a frame; the per-unit branch converts each bound against the definition it tests. Bounds that really are reversed still raise. - Unit normalization is restricted to numeric envelopes. Time-like ones are canonical nanoseconds whatever unit the coordinate names, so there is nothing to re-spell, and converting them as floats raised. - A row states no unit as NaN rather than None, and NaN never equals itself, so every member of a unitless partition looked like a unit mismatch and lost its read hint. One helper now reads an absent unit the same way everywhere it is consulted.
09a9ccb to
f0ab669
Compare
A relative bound measures against a span pooled from the members' magnitudes, so an archive whose files disagree on a unit gets the same treatment there as it does for bare ranges and sorting.
1 participant
Description
Closes #863.
The unreleased index rewrite generalized indexing from
timealone to every coordinate, and converted each numeric envelope to base SI on the way in while storing the base unit in a column documented as holding the original. Soget_contents()described envelopes no patch it yields actually has — a degrees coordinate read as radians,x_min = -2.042for a patch whosexis-117.0— and a bare numeric range meant SI at the spool while the same range on the patch meant the coordinate's own units, sospool.select(x=(-117.0, -116.9))silently returned nothing wherepatch.selectkept every channel.Both are confined to the unreleased window. On the last release the index describes only
time, so a range on any other dimension fell through to the patch at load and read natively, and there were nodistancecolumns to misreport. This restores the released meaning rather than choosing a new one.Envelopes are now stored in each coordinate's original units beside the original unit string:
Patch.select— for selection, chunk lengths, and residual trims alike.get_contents()shows native values beside new public{name}_unitscolumns, sodistance_min = 65.6is self-explaining.coord_defssemi-join, so candidacy stays one indexed scan rather than a per-unit fan-out. A quantity length is measured as a length, so 20 °C of extent is 36 °F.In an archive whose files state different units for one coordinate, a bare range selects a per-file native interval, sorting orders native magnitudes, and a relative offset measures against a pooled span. That is the accepted consequence of matching per-patch semantics: a spool has no single unit to read a bare number in. Archives whose files agree on a unit are unaffected.
This lands before the inventory's phase 3 (#857), whose inventory-backed
selectbuilds on this machinery — geometry axes arrive in the CRS's own units, exactly the case the SI reading got wrong.The changelog entry from #855 describing a chunk fix for the same unreleased regression is dropped: its net effect for users is zero and its closing clause is no longer true.
Filed while verifying, and deliberately not fixed here because both reproduce unchanged on dev: #870 (chunk lengths that are not a multiple of the sample step drop one sample per boundary) and #871 (chunk → select → re-chunk over-includes and duplicates samples).
Changelog
get_contents()shows them beside new{name}_unitscolumns, so a spool reportsdistanceand every other coordinate rather than only time (#863).chunk(distance=100)on a feet coordinate is 100 feet while100 * dc.units.mconverts per file.Checklist
I have (if applicable):