fix: update attrs and dims when squeezing patches (#623) - #712
Open
gaoflow wants to merge 1 commit into
Open
Conversation
Contributor
📝 WalkthroughWalkthrough
ChangesEmpty and 0-D Patch Shape Fixes
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_proc/test_proc_coords.py (1)
684-698: ⚡ Quick winAdd a direct regression for full
(1,1) -> 0-Dsqueeze.Line 695 currently tests single-dimension squeeze only. Add a
patch.squeeze()assertion path to verifydims == (),attrs.dim_tuple == (), andshape == (), which is the core behavior this fix targets.Suggested test extension
def test_squeeze_single_dim_on_1x1_patch(self): @@ out = patch.squeeze("distance") assert out.dims == ("time",) assert out.attrs.dim_tuple == ("time",) assert out.shape == (1,) + + out_all = patch.squeeze() + assert out_all.dims == () + assert out_all.attrs.dim_tuple == () + assert out_all.shape == ()🤖 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_proc/test_proc_coords.py` around lines 684 - 698, The test method test_squeeze_single_dim_on_1x1_patch currently only validates squeezing a single dimension. Add additional assertions to verify the full squeeze behavior (0-D array case) by calling patch.squeeze() without arguments on the original patch data and asserting that dims equals empty tuple (), attrs.dim_tuple equals empty tuple (), and shape equals empty tuple (). This provides regression coverage for the complete dimension elimination case that the fix addresses.
🤖 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/utils.py`:
- Around line 167-180: The IndexError exception handler at line 178 sets
data=None, which creates an invalid input for the dc.Patch constructor call at
line 180, causing the recovery to still fail. Instead of assigning data=None in
the except IndexError block, fall back to extracting the full data array from
patch_group["data"] similar to the else branch behavior (using
patch_group["data"][:]), which provides a valid recovery path while maintaining
the same contract for creating the Patch object with valid data, coords, dims,
and attrs parameters.
---
Nitpick comments:
In `@tests/test_proc/test_proc_coords.py`:
- Around line 684-698: The test method test_squeeze_single_dim_on_1x1_patch
currently only validates squeezing a single dimension. Add additional assertions
to verify the full squeeze behavior (0-D array case) by calling patch.squeeze()
without arguments on the original patch data and asserting that dims equals
empty tuple (), attrs.dim_tuple equals empty tuple (), and shape equals empty
tuple (). This provides regression coverage for the complete dimension
elimination case that the fix addresses.
🪄 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: a5113984-fb62-4a19-9909-9e9fe8421300
📒 Files selected for processing (6)
dascore/core/coordmanager.pydascore/io/dasdae/utils.pydascore/proc/coords.pydascore/utils/attrs.pytests/test_core/test_coordmanager.pytests/test_proc/test_proc_coords.py
gaoflow
force-pushed
the
fix-squeeze-attrs
branch
from
June 19, 2026 20:30
29aa430 to
691b8b2
Compare
Contributor
Author
|
Addressed the CodeRabbit follow-up in the latest push.
Verified locally:
|
Squeezing a (1,1) patch raised AssertionError because attrs
dim_tuple was not updated to match squeezed coords. The root
cause was twofold:
1. squeeze() called self.new() without dims/attrs, so update()
used old attrs which preserved the original dim_tuple
2. separate_coord_info()'s _get_dims treated empty string/empty
tuple dims as falsy, preventing dims from being updated to
an empty tuple when all dimensions are squeezed
- Pass dims and updated attrs from squeeze to Patch.new()
- Fix _get_dims to distinguish empty dims ('', ()) from no
dims info (None), using 'in' check instead of truthiness
- Fix CoordManager.shape to return () for truly empty dims,
not (0,) (which represents 1-D empty, not 0-D scalar)
- Fix CoordManager.validate_data default for 0-D shape
- Handle 0-D data in DASDAE writer/reader
Fixes DASDAE#623
gaoflow
force-pushed
the
fix-squeeze-attrs
branch
from
July 26, 2026 17:09
691b8b2 to
e00fe15
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/utils.py`:
- Around line 196-210: Update the data-loading flow around coords.select and
_read_data_array so IndexError from coordinate selection is never swallowed.
Limit the scalar-read fallback to errors from the initial data read, or
materialize the data and retry coords.select while allowing any second selection
error to propagate; preserve the caller’s filtering arguments and avoid
returning unfiltered data after selection failure.
🪄 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 Plus
Run ID: ff72838e-abc5-4ee8-8e33-24bf56e4a2bc
📒 Files selected for processing (6)
dascore/core/coordmanager.pydascore/io/dasdae/utils.pydascore/proc/coords.pydascore/utils/attrs.pytests/test_core/test_coordmanager.pytests/test_proc/test_proc_coords.py
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/test_core/test_coordmanager.py
- dascore/core/coordmanager.py
- dascore/proc/coords.py
- dascore/utils/attrs.py
- tests/test_proc/test_proc_coords.py
Comment on lines
+196
to
+210
| try: | ||
| if kwargs: | ||
| # We need to remove any coordinates from kwargs that are multi-dim | ||
| # coords. | ||
| cmap = coords.dim_map | ||
| sub_kwargs = { | ||
| i: v | ||
| for i, v in kwargs.items() | ||
| if (i not in cmap) or (len(cmap[i]) == 1) | ||
| } | ||
| coords, data = coords.select(array=patch_group["data"], **sub_kwargs) | ||
| else: | ||
| data = _read_data_array(patch_group["data"]) | ||
| except IndexError: | ||
| data = _read_data_array(patch_group["data"]) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not swallow IndexError from coordinate selection.
This try block covers both coords.select(...) and HDF5 data loading. If selection raises IndexError, the handler returns the original coordinates with the complete, unfiltered data, silently ignoring the caller’s selector. Restrict the scalar-read retry to the data-read operation, or retry selection with materialized data and let a second selection error propagate.
Proposed fix
- try:
- if kwargs:
+ if kwargs:
cmap = coords.dim_map
sub_kwargs = {
i: v
for i, v in kwargs.items()
if (i not in cmap) or (len(cmap[i]) == 1)
}
- coords, data = coords.select(array=patch_group["data"], **sub_kwargs)
- else:
- data = _read_data_array(patch_group["data"])
- except IndexError:
- data = _read_data_array(patch_group["data"])
+ try:
+ coords, data = coords.select(
+ array=patch_group["data"], **sub_kwargs
+ )
+ except IndexError:
+ data = _read_data_array(patch_group["data"])
+ coords, data = coords.select(array=data, **sub_kwargs)
+ else:
+ data = _read_data_array(patch_group["data"])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try: | |
| if kwargs: | |
| # We need to remove any coordinates from kwargs that are multi-dim | |
| # coords. | |
| cmap = coords.dim_map | |
| sub_kwargs = { | |
| i: v | |
| for i, v in kwargs.items() | |
| if (i not in cmap) or (len(cmap[i]) == 1) | |
| } | |
| coords, data = coords.select(array=patch_group["data"], **sub_kwargs) | |
| else: | |
| data = _read_data_array(patch_group["data"]) | |
| except IndexError: | |
| data = _read_data_array(patch_group["data"]) | |
| if kwargs: | |
| # We need to remove any coordinates from kwargs that are multi-dim | |
| # coords. | |
| cmap = coords.dim_map | |
| sub_kwargs = { | |
| i: v | |
| for i, v in kwargs.items() | |
| if (i not in cmap) or (len(cmap[i]) == 1) | |
| } | |
| try: | |
| coords, data = coords.select( | |
| array=patch_group["data"], **sub_kwargs | |
| ) | |
| except IndexError: | |
| data = _read_data_array(patch_group["data"]) | |
| coords, data = coords.select(array=data, **sub_kwargs) | |
| else: | |
| data = _read_data_array(patch_group["data"]) |
🤖 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 `@dascore/io/dasdae/utils.py` around lines 196 - 210, Update the data-loading
flow around coords.select and _read_data_array so IndexError from coordinate
selection is never swallowed. Limit the scalar-read fallback to errors from the
initial data read, or materialize the data and retry coords.select while
allowing any second selection error to propagate; preserve the caller’s
filtering arguments and avoid returning unfiltered data after selection failure.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #623 —
.squeeze()on a (1,1) patch raisesAssertionError: dim mismatch on coords and attrs.Root cause
Two issues:
squeeze()calledself.new(data=data, coords=coords)without passingdimsorattrs, soupdate()fell through toPatchAttrs.update(coords=coords)which usesseparate_coord_infowith the OLD dims — the old dims leaked through.separate_coord_info's_get_dimstreated empty string ("") and empty tuple (()) as falsy, same asNone, so it could not distinguish "dims are empty" from "no dims info found". When the squeezed CoordManager reported empty dims(), the dims update was silently skipped.Changes
dascore/proc/coords.py:squeeze()now passesdims=coords.dimsandattrs=self.attrs.new(dims=...)toself.new()dascore/utils/attrs.py:_get_dims()uses"dims" in objinstead of truthiness check, returnsNonewhen dims can't be determined (vs()when dims are empty); condition at line 292 usesis not Nonecheckdascore/core/coordmanager.py:shapeproperty returns()for truly empty dims (0-D scalar),(0,)for 1-D empty;validate_datausesnp.empty(self.shape)for None data to match coord shapedascore/io/dasdae/utils.py: Writer handles 0-D data arrays; reader handles missing data node gracefullyTest plan
test_squeeze_single_dim_on_1x1_patchSummary by CodeRabbit
shapeand default data initialization.dimsand related metadata consistently.dimshandling.