-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add RoundMode
(half even; half away from zero) and fix inconsistent rounding between float and decimal
#21883
base: main
Are you sure you want to change the base?
feat: Add RoundMode
(half even; half away from zero) and fix inconsistent rounding between float and decimal
#21883
Conversation
…istent rounding between float and decimal
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #21883 +/- ##
==========================================
- Coverage 80.92% 80.91% -0.02%
==========================================
Files 1624 1624
Lines 234643 234694 +51
Branches 2693 2693
==========================================
+ Hits 189878 189893 +15
- Misses 44133 44169 +36
Partials 632 632 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I think we need to do another pass over the names. I think Can we discuss that in the issue? Also, please don't make me catch API changes in the review, please just stick with what the issue suggests or discuss changes in the issue first. |
@orlp as discussed I adjusted the names and implemented
please have a look 🤓 |
match mode { | ||
RoundMode::HalfToEven => "half_to_even", | ||
RoundMode::HalfAwayFromZero => "half_away_from_zero", | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the enum is annotated with strum
, we can just use:
Into::<&str>::into(mode)
here, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please bump the minor IR version in visit.rs
(in the impl of NodeTraverser
).
#[pyclass(name = "RoundMode")] | ||
pub struct PyRoundMode { | ||
inner: RoundMode, | ||
} | ||
|
||
#[pymethods] | ||
impl PyRoundMode { | ||
#[getter] | ||
fn kind(&self) -> &str { | ||
self.inner.into() | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then there is no need for these.
} else { | ||
// Note we do the computation on f64 floats to not lose precision | ||
// when the computation is done, we cast to f32 | ||
let multiplier = 10.0.pow(decimals as f64); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use powi
without casting the exponent. This applies to the other spots too.
.apply_values(|v| { | ||
// We use rounding=ROUND_HALF_EVEN | ||
let res = match mode { | ||
RoundMode::HalfToEven => ca.apply_values(|v| { | ||
let rem = v % multiplier; | ||
let is_v_floor_even = ((v - rem) / multiplier) % 2 == 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please avoid the extra division by doing a modulo with 2 * multiplier
and some extra logic? i128
division/modulo is very very expensive.
fix #21800 (though as @orlp mentioned there are theoretically more round modes to add)
Current Problem⚠️
round
expression has nomode
and is inconsistentDecimal
: uses "half to even"Float
: uses "half away from zero"PR Solution 🍀
Decimal
: implement "half away from zero"Float
: implement "half to even"Example
Open
IMO having consistent rounding across types and the two most used round modes will already be a great benefit and cover 99% of use-cases.
We can implement the other modes in the future if time and demand is there.