Convert plan trims to each patch coordinate's own units - #838
Conversation
|
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 (7)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe patch loader converts canonical SI trim bounds into each coordinate’s native units before selection. Shared range utilities now support this conversion for catalog residual selection. Tests cover feet, SI, and unitless coordinates. ChangesNon-SI coordinate trimming
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_core/test_patch_chunk.py (1)
910-924: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise trimmed SI and unitless coordinates.
chunk(distance=None)keeps the full source envelope._load_trimmed_patchthen skipsPatch.selectbecause the member limits are not modified. These tests cannot detect a regression in_as_native_unitsfor SI or unitless coordinates.Use a finite chunk size such as
distance=100withkeep_partial=Truein both tests. Assert complete sample coverage after the trim path runs.🤖 Prompt for 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. In `@tests/test_core/test_patch_chunk.py` around lines 910 - 924, Update test_si_coord_unchanged and test_unitless_coord_unchanged to call spool.chunk with a finite distance=100 and keep_partial=True instead of distance=None. Retain the existing total sample-coverage assertions so both tests exercise _load_trimmed_patch and validate complete coverage after trimming.
🤖 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.
Nitpick comments:
In `@tests/test_core/test_patch_chunk.py`:
- Around line 910-924: Update test_si_coord_unchanged and
test_unitless_coord_unchanged to call spool.chunk with a finite distance=100 and
keep_partial=True instead of distance=None. Retain the existing total
sample-coverage assertions so both tests exercise _load_trimmed_patch and
validate complete coverage after trimming.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2efd9957-d695-4569-9127-4b63ca1ea613
📒 Files selected for processing (3)
dascore/utils/patch_assembly.pydocs/changelog.qmdtests/test_core/test_patch_chunk.py
326d464 to
017bb54
Compare
![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: 326d464387
ℹ️ 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".
| # are valid patch selections. | ||
| if select_kwargs := _coord_only_kwargs(patch, source_kwargs): | ||
| patch = patch.select(**select_kwargs) | ||
| patch = patch.select(**_as_native_units(patch, select_kwargs)) |
There was a problem hiding this comment.
For file-backed spools with a non-SI numeric coordinate, the conversion here occurs only after load_patch(kwargs), but PlanResolver._load_member forwards the original canonical-SI bounds through FileResolver to dc.read, and readers such as H5Simple and DASDAE apply those bare bounds while loading. A feet-backed file is therefore first truncated as though meter magnitudes were feet; this later native-unit selection cannot recover the discarded samples and can produce short or empty chunks. The plan trim must be withheld from the read hint, or converted before the reader applies it.
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #838 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 175 176 +1
Lines 18475 19372 +897
==========================================
+ Hits 18475 19372 +897
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:
|
017bb54 to
4707f90
Compare
Coordinate envelopes are stored in the index as canonical SI, so a chunk plan hands its member trims down as SI magnitudes. Those were applied to the patch coordinate as bare numbers, which means a trim of 20 to 60 on a coordinate stored in feet cut 20 to 60 feet rather than the 20 to 60 metres the plan described. Every piece covered the wrong physical interval and the samples between them were dropped: chunking a 300 channel patch whose distance is in feet returned 61 channels. Convert at load, where the coordinate's own unit is finally known. Spool.select already did this through _CanonicalRange; the plan path had no equivalent step, so the two disagreed about the same physical range on the same spool. Both now go through one helper, express_range_for_coord, whose bare_is_si flag names the one way they legitimately differ: a plan trim's plain numbers are SI magnitudes and must convert, while a residual selection's plain numbers are sample indices and must not. Move that helper and the _CanonicalRange machinery it needs from dascore.io.index.catalog to dascore.utils.misc, alongside the other range helpers. The assembler lives in utils and would otherwise have to import private names from io, which is backwards.
4707f90 to
acad4de
Compare
Resolves conflicts from dev's PLC0415 enablement, the chunk-by-size schema work, and the test-import cleanup: - catalog.py keeps dev's SPOOL_PRIVATE_RENAMES import and drops the now-unused module-level get_quantity, since _CanonicalRange moved to dascore.utils.misc on this branch. - misc.py's deferred get_quantity import carries an explicit noqa: PLC0415 (dascore.units imports misc at import time). - The index tests take dev's module-level imports and its 'is not None' narrowing, retargeted at dascore.utils.misc. - Both changelog entries are kept.
1 participant
Description
Spool.selectandSpool.chunkdisagree about what a plan trim means on a coordinate whose units are not the canonical SI unit.selectis correct;chunksilently drops data.The index stores coordinate summaries in canonical SI, but never rewrites the patch — the patch keeps its native units.
docs/notes/spool_selection.qmdmakes that the contract:selecthonors the second sentence through_CanonicalRange.for_patch_coord, which converts the range once the patch's own unit is known. The plan path had no equivalent step: it handed SI magnitudes straight topatch.select(...)as bare numbers, so a trim of20to60cut 20–60 feet from a coordinate stored in feet rather than the 20–60 metres the plan described. Each piece covered the wrong physical interval and the samples between pieces were dropped.This adds that deferred conversion to the assembler, reusing
_CanonicalRangerather than introducing a second implementation. Unitless coordinates keep their bare canonical magnitudes, which is the documented policy for them.Scope of exposure
No IO reader produces non-SI coordinate units, so this is not reachable by reading a file. It requires an explicit
convert_units/set_units, which is public and documented (docs/tutorial/concepts.qmd), and which the selection spec already uses as its own worked example (docs/notes/spool_selection.qmd, plus theselectfixture intests/test_core/test_spool_select_spec.py). So: a real contract violation and silent data loss, but on a path a user has to opt into rather than one every archive hits.Found while building
Spool.unselect, which inherits the same plan machinery. Split out so it lands on its own merits andchunkgets it independently.Tests
TestNonSIUnitTrimintests/test_core/test_patch_chunk.py. Three of the five fail without the change (sample count, per-piece agreement with the plan, physical span of a piece); the other two are SI and unitless controls that must keep passing either way.Changelog
none
Checklist
I have (if applicable):
Summary by CodeRabbit
Bug Fixes
Documentation