Treat patch attrs and coords as fully independent - #757
Conversation
Attrs and coords no longer interact anywhere in DASCore. Attr names that look like flat coordinate metadata (channel_step, pulse_len, gain_max, ...) are ordinary attrs on every entry point: PatchAttrs construction, subclass fields, update_attrs, DASDAE round-trips, and the spool index. Coordinates are never inferred from attr names. - Delete separate_coord_info and its greedy suffix/dims inference, the _raise_if_coord_attr_updates guard, and is_valid_coord_str. - Delete the attrs branch of get_coord_manager (a coord-shaped attr could crash it via a phantom single-field coord) and the unused CoordManager.update_from_attrs. - Quarantine all legacy DASDAE metadata handling in a dasdae-private _compat module; legacy flat coord keys are stripped against the file's actual stored coord names, never by key shape. New files carry a root marker declaring the attr namespace pure so coord-shaped attrs round-trip exactly. - Index every scalar attr; drop the catalog-wide envelope-name reservation and its warn-and-drop. The one surviving check: when the flat get_contents frame is built, an attr whose name equals a coordinate envelope column warns once and the coord wins the column; the attr stays queryable via the _attrs namespace. - Stop the TDMS reader from mirroring coord metadata into attrs (caught by the new FiberIO hygiene test). - Add regression tests for every failure mode of the old suffix-capture behavior, a parametrized hygiene test over all shipped readers, and an import-graph test pinning _compat inside dascore/io/dasdae.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughCoordinate-shaped attributes remain independent from coordinate construction, DASDAE legacy metadata is normalized during reads, and index flat views now resolve attribute/envelope collisions in favor of coordinate columns while retaining ChangesCoordinate metadata independence
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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
added
IO
![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: 24ef94fad0
ℹ️ 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".
| has_legacy_patches = ( | ||
| waveforms is not None | ||
| and len(waveforms) | ||
| and not hfile.attrs.get(_SEPARATE_ATTRS_KEY, False) | ||
| ) | ||
| if not has_legacy_patches: | ||
| hfile.attrs[_SEPARATE_ATTRS_KEY] = True |
There was a problem hiding this comment.
When H5Writer appends to an existing unmarked legacy file, this leaves the file-wide marker unset, but the newly written groups already use the separated format. Subsequent reads therefore treat those new groups as legacy and strip_legacy_coord_fields removes legitimate attrs such as time_step whenever the group has a matching coordinate, breaking the promised round trip. The format distinction needs to be recorded per group, or new groups in mixed files must otherwise be exempted from legacy stripping.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in aede734. The separated-attrs marker is now stamped on each patch group as well as the file root, and the read/scan paths check _is_legacy_group (file legacy AND group unmarked). Groups appended to a legacy file keep exact attr round-trips while the pre-existing groups still get legacy stripping; test_append_to_legacy_file_keeps_new_attrs covers the mixed-file case end to end.
| df = self._pivot_coords(df) | ||
| df = self._apply_attr_columns(df, attr_columns) | ||
| if residuals: | ||
| df = apply_residuals(df, residuals) |
There was a problem hiding this comment.
When an explicitly namespaced attr collides with a coordinate envelope and is queried with a compiled regex, _apply_attr_columns omits the attr before apply_residuals runs. The residual then reads the coordinate-backed flat column of the same name rather than the attr value, so _attrs={"channel_min": re.compile(...)} can incorrectly return no patches or match based on coordinate metadata even though the comment says the attr remains queryable through _attrs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in aede734. apply_residuals now takes the original attr series and prefers them over same-named flat columns. Worth noting the scenario is narrower than described: the SQL attr clause pre-filters to rows that have the attr, so a cross-patch collision disappears from the filtered frame — the bug only bit when a single patch carried both the coord and the same-named attr. That case is pinned by test_regex_query_reads_attr_under_collision (verified failing against the old behavior).
| omitted from the flat view (still queryable via the _attrs | ||
| namespace) with a warning. | ||
| """ | ||
| clobbered = frozenset(name for name in new_columns if name in out.columns) |
There was a problem hiding this comment.
For patches without a time or distance coordinate, attrs such as time_step should be ordinary flat attrs, but the patches table always projects nullable time_* and distance_* columns. Checking only name in out.columns therefore classifies these attrs as collisions, warns, and suppresses their values even though no matching coordinate exists in the result. Collision detection should consult the pivoted coordinate membership rather than mere presence of the fixed schema columns.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in aede734, via reservation rather than membership detection: time_min/max/step and distance_min/max/step are now in RESERVED_ATTR_COLUMNS, so attrs with those names warn and are skipped at ingest like other structural storage columns. These six exist on the patches table for every row regardless of the patch's coords, and letting an attr fill them would make downstream consumers (e.g. plan output records) misread attr values as structural envelopes. Every other coord's envelope columns only exist when the coord does, so name in out.columns is a true membership test there and the collision-warning path is unchanged. test_fixed_envelope_attr_warns_and_skips covers it.
| # Structural storage/contract column names cannot double as attr | ||
| # columns. Coordinate-envelope-shaped names (e.g. "channel_step") | ||
| # are ordinary attrs; a genuine collision with a coord envelope | ||
| # column is handled (warn, coord wins) when the flat view is built. | ||
| if sanitize_attr_name(name) in RESERVED_ATTR_COLUMNS: |
There was a problem hiding this comment.
Allowing names such as channel_step into the index exposes an incompatible derived-catalog path: dascore/io/index/planned.py::_output_records still discards every public key ending in _min, _max, _step, or _units, regardless of whether that key belongs to a coordinate. Consequently, after operations that create a derived spool, such as spool.chunk(time=...) or concatenation, an ordinary channel_step attr with no channel coordinate disappears from get_contents() and is no longer selectable through _attrs; the derived-record filter must distinguish actual coordinate envelopes from ordinary attrs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in aede734. _output_records now skips only envelope keys of coords actually present in the row (dims, aux coords, and _{name}_def_key-derived names, plus the fixed time/distance columns) instead of anything suffix-shaped, so a channel_step attr with no channel coordinate survives chunk/concat into the derived catalog. test_chunk_preserves_coord_shaped_attr covers get_contents and _attrs selection after spool.chunk.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #757 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 161 162 +1
Lines 16619 16544 -75
=========================================
- Hits 16619 16544 -75
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:
|
![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
🧹 Nitpick comments (1)
tests/test_core/test_coordmanager.py (1)
912-931: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSwap test names to match their behavior.
The test names and docstrings for updating
minandmaxare swapped relative to their implementations.test_update_mintests the{dim}_maxkwarg, whiletest_update_maxtests the{dim}_minkwarg.Please swap their names (and docstrings) to reflect what is actually being tested.
♻️ Proposed fix
- def test_update_min(self, cm_basic): - """Ensure a {dim}_max kwarg updates the appropriate coord.""" + def test_update_max(self, cm_basic): + """Ensure a {dim}_max kwarg updates the appropriate coord.""" for dim in cm_basic.dims: coord = cm_basic.coord_map[dim] new = cm_basic.update(**{f"{dim}_max": coord.min()}) new_coord = new.coord_map[dim] assert len(new_coord) == len(coord) assert new_coord.max() == coord.min() - def test_update_max(self, cm_basic): - """Ensure a {dim}_min kwarg updates the appropriate coord.""" + def test_update_min(self, cm_basic): + """Ensure a {dim}_min kwarg updates the appropriate coord.""" for dim in cm_basic.dims: coord = cm_basic.coord_map[dim] dist = coord.max() - coord.min() new = cm_basic.update(**{f"{dim}_min": coord.max()})🤖 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_coordmanager.py` around lines 912 - 931, Swap the names and docstrings of test_update_min and test_update_max so each accurately describes its tested keyword: the `{dim}_max` behavior belongs to test_update_max, and the `{dim}_min` behavior belongs to test_update_min. Leave the test implementations unchanged.
🤖 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/dasdae/_compat.py`:
- Around line 53-78: In the legacy string-coordinate handling block, check
get_config().allow_dasdae_format_unpickle before calling pickle.loads, and skip
deserialization entirely when disabled; preserve decoding only for explicitly
enabled compatibility. Update
test_translate_legacy_attrs_ignores_non_mapping_coords to assert that
pickle.loads is not called under the default configuration.
In `@dascore/io/index/backend.py`:
- Around line 852-881: Update the collision warning in _apply_attr_columns to
pass stacklevel=2, matching the warning behavior in _ensure_attr_columns so it
points to the caller rather than the internal frame.
---
Nitpick comments:
In `@tests/test_core/test_coordmanager.py`:
- Around line 912-931: Swap the names and docstrings of test_update_min and
test_update_max so each accurately describes its tested keyword: the `{dim}_max`
behavior belongs to test_update_max, and the `{dim}_min` behavior belongs to
test_update_min. Leave the test implementations unchanged.
🪄 Autofix (Beta)
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
Run ID: 5bdc9e67-e642-42b2-b9d5-3bd8d2af2dc8
📒 Files selected for processing (21)
dascore/core/attrs.pydascore/core/coordmanager.pydascore/io/dasdae/_compat.pydascore/io/dasdae/core.pydascore/io/dasdae/utils.pydascore/io/index/backend.pydascore/io/index/ingest.pydascore/io/tdms/utils.pydascore/proc/basic.pydascore/utils/attrs.pydascore/utils/misc.pydocs/notes/spool_index.qmdtests/test_core/test_attrs.pytests/test_core/test_coordmanager.pytests/test_core/test_patch.pytests/test_integrations/test_attr_coord_independence.pytests/test_io/test_common_io.pytests/test_io/test_dasdae/test_dasdae.pytests/test_io/test_index/test_index_edge_cases.pytests/test_utils/test_attrs_utils.pytests/test_utils/test_coordmanager_utils.py
💤 Files with no reviewable changes (4)
- dascore/io/tdms/utils.py
- dascore/core/attrs.py
- dascore/proc/basic.py
- dascore/utils/misc.py
Its only production caller was the TDMS coord-metadata mirroring removed
in the previous commit; the method existed solely to produce flat
{name}_{field} attr mirrors, which no longer have any consumer.
coderabbitai
Bot
added
documentation
|
✅ Documentation built: |
- Check the unpickle opt-in gate before pickle.loads ever runs; a malicious legacy coords payload previously executed during the decode that preceded the gate. Regression test proves no decode happens without opt-in. - Stamp the separated-attrs marker per patch group as well as per file, so groups appended to a legacy file are not legacy-stripped on read (their coord-shaped attrs now round-trip exactly). - Reserve the six fixed time/distance envelope columns at ingest: they exist on the patches table for every row regardless of coords, so a same-named attr could not be told apart from structural metadata downstream. Other coords' envelopes keep the collision-warning path. - Preserve envelope-shaped attrs through derived catalogs: plan output records now skip only envelope keys of coords actually present in the row instead of anything suffix-shaped, so chunk/concat keep attrs like channel_step queryable. - Evaluate regex attr residuals against the original attr values, which the flat column no longer holds when a patch has both a coord and a same-named attr. - Initialize the collision warn-once set in the backend constructor, add stacklevel to the warning, share the legacy flat-field constant, and unswap two coordmanager test names.
coderabbitai
Bot
added
spool
Attrs and coords are independent as of #757, so the channel_stride stopgap is no longer needed; the attr keeps the vendor's channel_step name and a test pins that it survives indexing. Review fixes: - Discard unknown fields after scan-mode parsing. Omitting the sample declarations stopped them being decoded but protobuf still retained their raw bytes, so a metadata-only scan held ~99% of the recording in memory. - Label complex FFT packets fourier_transform rather than power_spectral_density; complex coefficients are not real power. - Compare sample_count modulo 2**32 so a uint32 rollover mid-acquisition is not misread as a gap. - Raise a specific error for timeseries packets carrying samples in raw_frames instead of a confusing payload-size mismatch. - Restore dascore.io.sintela_binary as a forwarding module; it shipped in v0.1.18 and the move would have broken existing imports.
1 participant
Description
This PR makes patch attrs and coords fully independent: no code path infers coordinate structure from attr names anymore, and no attr name is reserved or rejected because it happens to look like coordinate metadata. It is a hard break on the dev line.
The problem (found via #656): any attr whose name ended in a
CoordSummaryfield (_min,_max,_step,_len,_dims,_fingerprint) was silently captured as coordinate metadata for a phantom coordinate named after the prefix.separate_coord_info({'channel_step': 3})invented achannelcoordinate, and passing the real dims did not protect callers because greedy inference overrode them. This produced four different behaviors for the same attr depending on the entry point (raise, silently accept, silently accept, warn-and-drop), silently dropped legitimate vendor metadata like Sintela'schannel_stepfrom the spool index, and could crashget_coord_manageroutright (get_coord(step=3)cannot build a coordinate from one field). The Sintela reader had to rename its attr tochannel_strideto dodge this.The rule now: attrs are an opaque, open namespace; coords are structural and live only in
CoordManager.update_attrs(channel_step=3)sets an attr, always, on every entry point, even when the patch has achannelcoordinate. Nothing is inferred from a name's shape. There are no runtime checks, with exactly one exception described below.What was deleted:
separate_coord_infoand its greedy suffix/dims inference,_raise_if_coord_attr_updates(soupdate_attrsno longer raisesPatchAttributeErrorfor coord-shaped names), andis_valid_coord_str.attrsbranch ofget_coord_manager(the crash path) andCoordManager.update_from_attrs(zero production callers)._is_envelope_shaped) and its warn-and-drop; every scalar attr is now indexed and queryable via the_attrsnamespace.What replaced it:
_compatmodule holding all legacy DASDAE metadata handling. Legacy files (which mixed flat coord keys into the stored attr namespace) are cleaned by stripping exact{coord}_{field}keys against the file's actually-stored coord names — never by key shape, so an old file with apulse_lenattr and nopulsecoord keeps its attr. New files carry a root marker (__attrs_coords_separate__) declaring the attr namespace pure, so even an attr shadowing the patch's own coord envelope (distance_stepalongside adistancecoord) round-trips exactly. An import-graph test pins_compatinsidedascore/io/dasdae/.get_contents()frame is materialized, an attr whose name equals a coordinate envelope column ({coord}_min/max/stepfor a coord present in that frame) is a genuine collision — the coord wins the bare column, the attr column is omitted, aUserWarningfires once per backend, and the attr stays fully queryable via_attrs. The check runs after coord envelope pivoting so cross-patch collisions (attr from one patch vs coord from another) are caught too.{coord}_{field}for a coord on the same patch). This immediately caught the TDMS reader doing exactly that; fixed here.get_coord_managercrash repro (now aTypeError), DASDAE round-trips including the self-shadowing case, index ingest without warnings, and the collision warning with cross-patch and negative-control variants.Follow-up (separate PR, after this lands): rebase #656 and revert Sintela's
channel_strideback to the vendor namechannel_step, which this PR makes representable end to end.Changelog
channel_step,gain_max, …) are ordinary attrs on every entry point; coordinates are never inferred from attr names, anddascore.utils.attrs.separate_coord_infoandCoordManager.update_from_attrsare removed.Checklist
I have (if applicable):
Summary by CodeRabbit
time_step,channel_min/max/step) are handled as regular patch attributes (not used to infer/update coordinates).{dim}_{field}kwargs._attrs._attrsaccess.