Allow scalar data for zero-dim CoordManager and propagate CoordManager dims in PatchAttrs.update#625
Allow scalar data for zero-dim CoordManager and propagate CoordManager dims in PatchAttrs.update#625d-chambers wants to merge 3 commits intomasterfrom
Conversation
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughPropagates CoordManager-provided dims into Patch attrs and uses them to derive coord parsing; adds an early return in CoordManager.validate_data to accept scalar data when no dims exist; adds tests for scalar/no-dim validation and squeeze-to-scalar behavior. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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
![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: d6e34dc6d9
ℹ️ 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".
dascore/core/attrs.py
Outdated
|
|
||
| coord_info, attr_info = separate_coord_info(kwargs, dims=self.dim_tuple) | ||
| coord_info, attr_info = separate_coord_info( | ||
| kwargs, dims=tuple(out.get("dims", self.dim_tuple)) |
There was a problem hiding this comment.
Wrapping out.get("dims", self.dim_tuple) in tuple(...) here regresses the common case where out["dims"] is a comma-separated string (e.g. "distance,time"), because it becomes a character tuple instead of dimension names. That corrupts dims during PatchAttrs.update and breaks coordinate-field updates such as attrs.update(distance_units="miles") (the distance coord is not updated and dims is rewritten as comma-joined characters).
Useful? React with 👍 / 👎.
| if not self.dims and data.shape == (): | ||
| return data |
There was a problem hiding this comment.
This early return allows scalar data for empty-dim managers, but CoordManager.shape still reports (0,) when self.dims is empty, so zero-dim patches can now be created with data.shape == () while patch.shape == (0,) and patch.size == 0. That violates the documented Patch shape/size behavior and can mislead downstream code that relies on coords.shape/coords.size after squeeze() or other zero-dim reductions.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dascore/core/attrs.py`:
- Around line 244-246: PatchAttrs.update is passing dims through tuple(...)
directly which turns comma-separated strings into character tuples and breaks
separate_coord_info's prefix matching; fix by normalizing dims first (handle
string vs iterable) before converting to a tuple — e.g., detect if
out.get("dims", self.dim_tuple) is a str and split on commas (or otherwise use
the existing iterate() defensive logic) to produce a sequence of full dim names,
then wrap that normalized sequence with tuple(...) when calling
separate_coord_info; update the call site in PatchAttrs.update where dims is
computed so separate_coord_info receives a tuple of full dimension names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e707e20c-2361-404f-ba21-d316464452d3
📒 Files selected for processing (4)
dascore/core/attrs.pydascore/core/coordmanager.pytests/test_core/test_coordmanager.pytests/test_proc/test_proc_coords.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #625 +/- ##
==========================================
+ Coverage 99.49% 99.93% +0.44%
==========================================
Files 135 135
Lines 11570 11576 +6
==========================================
+ Hits 11511 11568 +57
+ Misses 59 8 -51
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Motivation
CoordManagerwith no dimensions accepts scalar data arrays so zero-dim patches can be represented as scalars.PatchAttrsis updated with aCoordManager, the patch dimensions are updated and subsequent coordinate parsing uses the new dims.Description
PatchAttrs.update, ifcoordsis aCoordManagerthen setout["dims"] = passed_in_coords.dimsso the model reflects the incoming dims.separate_coord_infocall to usetuple(out.get("dims", self.dim_tuple))so coordinate extraction honors the updated dims.CoordManager.validate_data, allow scalar data for managers with no dims by returning the scalar whennot self.dimsanddata.shape == ().test_scalar_data_ok_for_no_dimsintests/test_core/test_coordmanager.pyandtest_squeeze_all_dims_len_oneintests/test_proc/test_proc_coords.pyto cover the new behaviors.Testing
test_scalar_data_ok_for_no_dimswhich asserts a zero-dimCoordManageraccepts scalarnp.array(1.0)and the test passed.test_squeeze_all_dims_len_onewhich asserts squeezing an aggregated (1,1) patch produces a scalar patch with no dims and the test passed.Codex Task
Summary by CodeRabbit
Bug Fixes
Tests