Streamline exact rolling output construction - #775
Merged
Conversation
The numpy roller allocated three arrays to pad and center its result, and both engines rebuilt the output Patch through Patch.update, which reconciles attrs against coords a second time even though the roller has already built both. _PatchRollerInfo also paid pydantic validation on every rolling call despite being an ephemeral internal object. - make _PatchRollerInfo a frozen dataclass - pad and center in a single allocation instead of pad + roll - reuse the patch coord manager when there is no step - construct the output Patch directly from the built coords and attrs Output is bit-for-bit unchanged: data, coords, attrs and raised errors match over a sweep of both engines, all reductions, apply, 1D/2D/3D, int/float32/float64, NaN and inf data, and windows of 1-11 with and without step and centering.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesRolling metadata and output handling
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
patch
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #775 +/- ##
=======================================
Coverage 99.93% 99.93%
=======================================
Files 145 145
Lines 12859 12864 +5
=======================================
+ Hits 12851 12856 +5
Misses 8 8
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/proc/rolling.py`:
- Around line 153-156: Update the rolling execution path around the raw result
conversion before _pad_roll_array: preserve the dtype returned by function, and
only promote or cast when required to represent NaN padding. Avoid forcing
float64 so float32 remains float32 and complex outputs retain their imaginary
components, while keeping _new_patch and the existing rolling flow unchanged.
In `@tests/test_proc/test_rolling.py`:
- Around line 291-295: Update test_attrs_conform_to_coords to parameterize the
rolling engine over both "numpy" and "pandas", pass the selected engine to
random_patch.rolling, and retain the existing attrs/coords assertions for each
parameterized case.
- Around line 260-267: Strengthen rolling metadata tests in
tests/test_proc/test_rolling.py at lines 260-267 and 284-289: update
test_units_preserved to include or iterate over a non-dimension coordinate and
verify its units after rolling, and update the later test to compare that
coordinate’s values and metadata with the input rather than only checking its
name and presence.
- Around line 276-282: Strengthen test_history_appended for both parametrized
engines by replacing the broad history[-1] substring check with an exact
assertion that the new entry ends with ".apply(mean)" or matches the fully
formatted expected history entry, while preserving the existing length
assertion.
🪄 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: e61e08eb-fbeb-4cc1-998b-0304450d4b32
📒 Files selected for processing (3)
benchmarks/test_patch_benchmarks.pydascore/proc/rolling.pytests/test_proc/test_rolling.py
- assert the exact history entry per engine - check units and values of non-dimensional coords, not just dims - exercise both engines for the non-dim coord and attrs/coords cases
4 tasks
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
Follow-up to #765 and #768, covering the "additional exact rolling cleanup" items: reduce the per-call overhead of
patch.rolling(...)without changing any numerics.Profiling a representative localized DAS patch (
(28, 600)float32, 10-sample centered distance window) showed that ~90% of a rolling mean was metadata and array bookkeeping rather than the reduction itself. Three things stood out:_PatchRollerInfoinherits from a pydantic model, so everypatch.rolling(...)call paid validation for an ephemeral, internal object._pad_roll_arrayallocated three arrays:astypethennp.padthennp.roll. Because the padding is all NaN, the roll is equivalent to writing the data at an offset, so one allocation suffices.Patch.update(data=..., coords=..., attrs=...). That callsCoordManager.update_from_attrs, and thenPatch.__init__reconciles attrs against coords again, so the coord summaries were rebuilt twice per call even though the roller had already produced a validCoordManagerandPatchAttrs.Changes:
_PatchRollerInfois now a frozen dataclass._pad_roll_arraypads and centers in a single allocation.get_coordsreturns the patch's coord manager unchanged when there is no step (the rolling dimension is unchanged in that case)._new_patchhelper constructs the output Patch directly from the coords and attrs the roller already built. Both engines use it.No public API or behavior change;
engine,step,center,overlapand all reductions behave exactly as before.Verification
Output was compared against
masterover a sweep of 3,536 combinations: both engines, all six reductions plusapply, 1D/2D/3D patches, int64/float32/float64 data, data containing NaN and ±inf, patches with units and with non-dimensional coords, windows of 1/2/3/11, with and without step and centering. Data bytes, dtype, shape, coords and serialized attrs are identical in every case, and the 182 combinations that raise produce the same exception and message.Timings
Wall clock per call, mean of 200 runs:
(28, 600)distance window 10, centered(28, 600)time window 10(28, 600)window 11, step 3, centered(300, 2000)time window 10The large-patch case gains less because it is dominated by the reduction itself rather than by overhead.
Checklist
I have (if applicable):
Summary by CodeRabbit