Add Patch.from_parts and stop reconciling attrs twice - #777
Conversation
Patch.__init__ reconciles attrs against coords unconditionally, which establishes an invariant every Patch satisfies: the coord summaries on attrs match the coord manager. Patch.update reconciles as well, then hands the result to the constructor, which does the same work again. Add a public Patch.from_parts for callers which already hold a conforming pair. It validates the data shape and the dimension names but trusts the coord summaries, so it costs a few microseconds instead of the 130-250 the reconcile does. Patch.update now uses it: once directly, when nothing that can affect coords or attrs changed, and once at the end, where the constructor would otherwise repeat the reconcile just performed. PatchAttrs._conform_to rebuilds attrs from a coord manager with model_copy rather than a dump and re-validate. Convert the call sites which pass attrs, and so pay the more expensive update_from_attrs, or which bypass update entirely: rolling (retiring the _new_patch helper added in #775), sobel_filter, notch_filter, differentiate, velocity_to_strain_rate, radians_to_strain, and the aggregator backing mean/max/min/std/sum/median/first/last. Patch.__init__ is unchanged apart from turning its dim-mismatch assert into a raise, so the two paths behave the same under -O. IO readers and the sites which rely on reconciliation to repair stale attrs keep the strict path. Output is unchanged: data, coords, attrs, dims, the read-only flag and raised errors all match master across 648 patch/operation combinations. patch.new(data=arr) 515 us -> 6 us patch.new(data, coords=cm) 503 us -> 76 us patch.abs() 957 us -> 407 us patch.select(...) 824 us -> 359 us patch + 1 1800 us -> 986 us patch.rolling(time=5).mean() 11.2 ms -> 9.4 ms dc.Patch(...) direct 252 us -> 251 us (control)
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 33 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling 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 (11)
✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #777 +/- ##
=======================================
Coverage 99.93% 99.93%
=======================================
Files 145 145
Lines 12864 12881 +17
=======================================
+ Hits 12856 12873 +17
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:
|
1 participant
Description
Follow-up to #775, which sped up rolling partly by adding a local
_new_patchhelper that bypassedPatch.update. That helper was a symptom; the redundancy is inupdateitself. This removes it, and generalizes the fix.Patch.__init__reconciles attrs against coords unconditionally, which establishes an invariant every Patch satisfies:Patch.updatealso reconciles, then hands the result to the constructor, which does the same work again. Andpatch.new(data=arr)— the single most common shape of call in the library — reconciles twice even though nothing that could affect coords or attrs has changed.The cost here is not pydantic validation, which is worth stating since it is the intuitive suspect.
PatchAttrs.model_construct(pydantic's own validation bypass) measures slower than validating, 99 µs against 34.5 µs, because it is pure Python where validation is Rust in pydantic-core. The cost is dascore's own reconciliation:separate_coord_info,model_dump,to_summary_dict, and oneCoordSummaryper coord.Changes
Patch.from_parts(data, coords, attrs)— a public classmethod for code which already holds a conforming pair. It validates the data shape and the dimension names, but trusts the coordinate summaries, so it costs a few microseconds rather than 130–250. The docstring documents the contract and the failure mode, including the trap that attrs built with an emptycoordsdo not conform and the dimension check will not catch it.PatchAttrs._conform_to(coords)— rebuilds attrs from a coord manager usingmodel_copyinstead of a dump and re-validate. Verified equal toattrs.update(coords=coords), with identicalmodel_dump(), across unchanged / decimated / dropped-coord / transposed coord managers. 132 µs → 38 µs.proc.basic.update— returnsfrom_partsdirectly whenattrs,dimsand a differingcoordsare all absent, and otherwise reconciles once and constructs without letting__init__repeat it.Call-site conversions — only where there is a real win, meaning sites that pass
attrs=(and so hit the more expensiveupdate_from_attrs) or that bypassupdateentirely: rolling (retiring_new_patchfrom #775),sobel_filter,notch_filter,differentiate,velocity_to_strain_rate,radians_to_strain, and_apply_aggregator, which backs every aggregation.Patch.__init__is unchanged apart from turning its dim-mismatchassertinto a raise, so both construction paths behave the same underpython -O.What is deliberately not converted
coords=but no attrs (select,decimate,resample,pad,transpose, …) gain nothing from an explicit call, since they would have to buildattrs.update(coords=cm)themselves. They speed up for free from_conform_to.data=-only sites (abs,taper,detrend, …) likewise need no source change.utils/array.py:197, backing every binary operator — the most tempting target, and currently unsafe. When two patches with differing history are combined,_merge_modelspopscoordsfrom both, returning attrs withcoords == {}whiledimsstill matches, so the dimension check would pass and we would silently emit patches violating the invariant. Needs_merge_modelsfixed first.dropna,convert_units,rename_coords,dft/idft/stft/istft,integrate,spectrogram,velocity_to_strain_rate_edgeless,concatenate_patches,stack_patches. They still get the double→single reconcile win with no source change. Worth a separate issue; several are latent bugs the moment anyone bypasses reconciliation.Verification
Output is unchanged. A sweep of 648 combinations — 8 patch kinds (including int64, float32, NaN, units, non-dimensional coords, custom attrs) × 81 operations — compared data bytes, shape, dtype, coords repr, serialized attrs, dims and the read-only flag against
master. All 648 identical, and the 72 that raise produce the same exception and message. The sweep covers every converted site, every excluded site, the full rolling matrix from #775, and binary ops between patches with differing attrs.A new
TestAttrsCoordsInvariantasserts the invariant across 20 operations. It is what makesfrom_partslegal, so if a future change breaks it, the test names the reason. Writing it caught thatp + p.update_attrs(tag="x")raisesIncompatiblePatchErroroutright — onlyhistoryanddimsdifferences are tolerated between operands — so the test uses the case that actually exercises the coord-dropping path.Also repairs
test_conflicting_attrs_coords_raises, which was decorated@pytest.fixtureand so had never been collected, and asserted a message that does not exist in the codebase.Timings
Default example patch, mean per call:
patch.new(data=arr)patch.new(data, coords=cm)patch.new(data, coords, attrs)patch.abs()patch.select(time=...)patch.mean("time")patch + 1patch.differentiate("time")patch.rolling(time=5).mean()dc.Patch(...)direct — controlThe control is deliberately unmoved; the strict constructor was left alone. Ops on the large example patch gain proportionally less because they are compute-dominated.
Checklist
I have (if applicable):