Handle pint quantities explicitly in to_float - #834
Conversation
|
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: 14 minutes 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?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 (4)
📝 WalkthroughWalkthroughThe PR adds Pint quantity handling to ChangesQuantity and unit handling
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 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
bug
Update after adversarial reviewReviewed alongside #833; two findings addressed in The notch-filter guard is broader than the changelog said, and its message was wrong. It fires on any quantity when the coordinate has no units, including dimensionless ones — but the message claimed the value was "unit-bearing", which is false for The quantity contract was invisible in the rendered API docs. It lived only in the private Verified cleanA reviewer instrumented Known, not fixed here
|
to_float rejected every dimensional quantity -- including the seconds quantity its own docstring implies should work -- while silently accepting every dimensionless one. The silent path is the dangerous half: pint's __float__ converts a dimensionless quantity to base units, and information's base unit is the bit, so to_float(25 * MB) returned 200000000.0 rather than raising. Register Quantity so a time quantity converts to its duration in seconds and everything else raises UnitError naming the explicit alternatives. This matches the function's actual domain: every existing overload converts datetime64 to seconds since epoch, timedelta64 to seconds, or passes a plain number through. The magnitude is recursed back through to_float so an array-valued quantity takes the array path and an integer magnitude still widens to float. Instrumenting the full test suite found exactly one Quantity reaching to_float, in a test asserting the bits behavior, so nothing depended on the old semantics. Also raise UnitError with an explanatory message when filtering a unitless coordinate with a unit-bearing value, rather than letting pint's DimensionalityError leak out of proc.filter.
Review found the guard's message claimed the value was "unit-bearing", which is wrong for a dimensionless quantity. Every quantity is rejected, including dimensionless ones: there is nothing to convert against, and the previous behavior read `20 %` as 0.2 Hz. Say that, test all three kinds, and describe the real scope in the changelog. Also surface the quantity contract on to_float's public docstring; the overload's own docstring is not rendered in the API docs.
9366ad3 to
4b688e8
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #834 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 164 164
Lines 17924 17936 +12
=========================================
+ Hits 17924 17936 +12
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 #833, split out so the index schema bump stays reviewable on its own.
to_floathandled pint quantities backwards: it rejected everything with dimensions — including the seconds quantity its own docstring implies should work — and silently accepted everything without.2 * sDimensionalityError2.02 * minDimensionalityError120.0[1., 2.] * minDimensionalityErrorarray([60., 120.])25 * MB200000000.0UnitError50%0.5UnitError10 * mDimensionalityErrorUnitErrorThe silent half is the dangerous one.
to_float's fallback isfloat(obj), and pint'sQuantity.__float__converts a dimensionless quantity to base units. Bytes are dimensionless in pint (byte = 8 * bit,bitdimensionless), so a data size came back as its count in bits — eight times too large, with no exception and a plausible-looking number. It was found while adding size-based chunking, where an 8× overestimate of a memory budget is exactly the failure the feature exists to prevent.Why reject rather than pick a conversion
Two alternatives were considered and rejected:
.magnitude) is unit-blind:to_float(10 * km)andto_float(10 * m)would both give10.0, while callers treat the result as a number in canonical units (proc/filter.pycompares it against Nyquist). It also makes50%→50.0, contradicting the percent-is-a-fraction convention inmaybe_convert_percent_to_fractionandcoords._get_compatible_value.Instead, treat time as the function's domain, which is what every existing overload already does (datetime64 → seconds since epoch, timedelta64 → seconds, plain numbers passed through). A metre or a megabyte has no float representation in that domain, so it raises
UnitErrornamingconvert_unitsandget_byte_countas the explicit alternatives.The magnitude is recursed back through
to_floatrather than returned directly, which is what makes array-valued quantities take the array path and an integer magnitude still widen to float.Is anything relying on the old behavior?
No. The full suite was run with
to_floatinstrumented to record every call receiving aQuantity: exactly one hit across 8382 tests, and it was the test asserting the bits behavior itself. The only semantic loss isto_float(get_quantity("50%")) == 0.5, which nothing used and which is far more likely a bug than an intent when it reaches a seconds-converter.Drive-by
patch.notch_filter(distance=5 * dc.units.m)on a coordinate with no units leaked pint'sDimensionalityErrorthroughproc/filter.py. It now raisesUnitError: Cannot filter 'distance' with 5 m: the coordinate has no units.... The genericto_floatmessage would have been misleading in a distance filter, hence the specific one.Changelog
to_floatconverts a time quantity to seconds and raisesUnitErrorfor every other quantity, instead of silently reducing a dimensionless one to base units (25 MBcame back as2e8, the count in bits); useconvert_unitsorget_byte_countfor an explicit conversion.UnitError; a dimensionless one was previously read as a bare number (20 %became 0.2 Hz), so a call that silently did the wrong thing now stops.Checklist
I have (if applicable):
Summary by CodeRabbit
New Features
Bug Fixes
UnitErrorinstead of proceeding silently or exposing a lower-level conversion error.Documentation