Add Patch.squeeze_coords - #995
Conversation
Convert non-dimensional coordinates which hold a single value into patch attributes, leaving length one dimensions to Patch.squeeze.
|
Warning Review limit reached
Next review available in: 23 minutes Limit details: You’ve used all 2 included reviews currently available. 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?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day 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 (3)
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 (5)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughChangesThe PR adds Coordinate squeezing
🚥 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #995 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 202 202
Lines 27343 27406 +63
=========================================
+ Hits 27343 27406 +63
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: 8d5affb0f3
ℹ️ 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".
| return _NO_VALUE | ||
| values = np.asarray(coord.values).reshape(-1) | ||
| value = values[0] | ||
| if values.size > 1 and not bool(np.all(values == value)): |
There was a problem hiding this comment.
When a non-dimensional CoordPartial is large, coord.values is a cheap broadcasted null view, but values == value creates and scans a coordinate-sized boolean array before the following null check rejects it. Calling the default squeeze_coords() on a patch with a large partial coordinate can therefore consume substantial memory or OOM even though the coordinate is known immediately to have no value; check the first value for nullness before performing the array-wide equality comparison.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e13be9d: the nullish check now runs before the array-wide comparison, so a partial coord is rejected off its first value without materializing a coord-sized boolean array.
| attrs = self.attrs.model_dump(exclude_unset=True) | values | ||
| return self.new(coords=new_coords, attrs=attrs) |
There was a problem hiding this comment.
When the input comes from a format that supplies a PatchAttrs subclass, such as ODH4PatchAttrs, dumping the attrs to a plain dict causes self.new to reconstruct them as the base PatchAttrs. Thus squeezing an otherwise ordinary coordinate silently discards the format-specific attrs type and its validation behavior; update the existing attrs model (or otherwise reconstruct through its concrete class) instead of passing a plain mapping.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ec7f3db: the merge now goes through PatchAttrs.update, which rebuilds through self.from_dict, so the concrete class is kept (Patch.update then passes a PatchAttrs instance straight through). Pinned by test_attrs_class_kept.
Refuse to squeeze a coordinate the patch is not free to state: a private one, one a transform needs to invert itself, one named for something the attrs already use, and values the attrs cannot hold (object, complex, null). Reuse PatchAttrs.update for the merge and get_coord's message for a missing name, and treat a None argument as no argument.
Check for a null value before the array-wide comparison, and pin that a format's own attrs class survives the squeeze.
1 participant
Description
Adds
Patch.squeeze_coords, which converts non-dimensional coordinates holding a single value into patch attributes.A coordinate whose values are all the same says one thing about the patch rather than one thing about each sample.
squeeze_coordsdrops each such coordinate and stores its value in the attrs under the coordinate's name:It complements
Patch.squeeze, which removes length one dimensions:squeeze_coordsnever touches a dimension, sopatch.squeeze_coords().squeeze()keeps what a baresqueeze()would throw away (dropping a dimension drops the coordinates which depend on it).Details:
CoordError. The unnamed sweep passes over those instead, and an empty selection (squeeze_coords([])) does nothing.np.datetime64; every other value is stored as a python scalar.coords,dims,history,patch_id,processing_id) cannot hold a coordinate's value —dimsis dropped byPatchAttrs.from_dict,coordsis refused there, and the last three are stamped over by thepatch_functiondecorator after the body returns. A coordinate named for one of them is skipped by the sweep and refused when named.tests/test_workflow/_calls.pygains a catalogued call, whichTestEveryPatchFunctionrequires of every patch function.Changelog
Patch.squeeze_coordsconverts non-dimensional coordinates which hold a single value into patch attributes.Checklist
I have:
docs/contributing/general_guidelines.qmd).I have (if applicable):
Summary by CodeRabbit
New Features
squeeze_coordsto convert single-value, non-dimensional coordinates into patch attributes.Documentation
Bug Fixes