-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
fix rounding issue with exponents in fmt #116301
Conversation
r? @cuviper (rustbot has picked a reviewer for you, use r? to override) |
library/core/tests/fmt/num.rs
Outdated
assert_eq!(format!("{:.2e}", 1925), "1.93e3" ); | ||
assert_eq!(format!("{:.2e}", 1945), "1.95e3" ); | ||
assert_eq!(format!("{:.2e}", 1965), "1.97e3" ); | ||
assert_eq!(format!("{:.2e}", 1985), "1.99e3" ); |
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.
Aren't we supposed to be rounding ties toward even? So this would be "1.98e3", and many others would round down too.
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.
Is round to even spec'd in a doc? I was trying to keep the same behavior as before using https://doc.rust-lang.org/std/fmt/#precision as a reference for desired behavior based on the discussion here: https://users.rust-lang.org/t/scientific-notation-decimal-places/99682/5
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.
Sort of -- it's specified for operations on the f32
page, and the f64
page just references that.
When the number resulting from a primitive operation (addition, subtraction, multiplication, or division) on this type is not exactly representable as f32, it is rounded according to the roundTiesToEven direction defined in IEEE 754-2008.
We do seem to do this for fractional formatting as well:
fn main() {
println!("{:.2e}", 1.625);
println!("{:.2e}", 16.25);
println!("{:.2e}", 162.5);
println!("{:.2e}", 1625);
println!("{:.2e}", 16250);
}
Note that my choice of input values are represented precisely at all levels, so there's no ambiguity that these should be ties. The first three do round down to even, but the last two are rounding up.
1.62e0
1.62e1
1.62e2
1.63e3
1.63e4
(although I tested that on the playground, not your code.)
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.
Ah, I see what you're saying. Since the bug submitted was the number of digits expressed, would it be ok for me to take out the assert!
s that enforce the non round-to-even behavior and then work on the rounding in a follow up PR?
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.
If you're willing to work on both, I'd rather do it all at once so they can land in the same release, and we only have to explain changes once. :)
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.
gotcha 🙂 i think what I just pushed rounds to evens on a tie correctly now
94ab4d2
to
1341e63
Compare
library/core/src/fmt/num.rs
Outdated
@@ -330,10 +330,17 @@ macro_rules! impl_Exp { | |||
if subtracted_precision != 0 { | |||
let rem = n % 10; | |||
n /= 10; | |||
let last_digit_odd = (n % 10) % 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.
You don't need to isolate the last digit to know that it's odd.
let last_digit_odd = (n % 10) % 2 != 0; | |
let last_digit_odd = n % 2 != 0; |
library/core/src/fmt/num.rs
Outdated
n += 1; | ||
// if the digit is rounded to the next power | ||
// instead adjust the exponent | ||
if n % 10 == 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.
Hmm, doesn't this only tell you that you overflowed the least significant digit? e.g. maybe n=129 rolled up to 130, but that doesn't need any adjustment. For a new power of 10, like n=99 to 100, maybe something like n.log10() > (n - 1).log10()
would work?
0647635
to
4e4c972
Compare
library/core/src/fmt/num.rs
Outdated
// round up last digit | ||
if rem >= 5 { | ||
// round up last digit, round to even on a tie | ||
if rem > 5 || (rem == 5 && n % 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.
Sorry I keep noticing things, but I think we're almost there...
The check for even here is missing whether we removed any additional non-zero digits earlier. e.g., for {:.1e}
I expect:
- 124999 => 1.2e5 (rounded down)
- 125000 => 1.2e5 (tie rounded down to even)
- 125001 => 1.3e5 (rounded up)
- This one gets treated like a tie right now and prints 1.2e5
- 134999 1.3e5 (rounded down)
- 135000 1.4e5 (tie rounded up to even)
- 135001 1.4e5 (rounded up)
I think subtracted_precision > 1
would account for the extra non-zero digits? Please do add these values as tests.
library/core/tests/fmt/num.rs
Outdated
assert_eq!(format!("{:.2e}", 1925), "1.92e3" ); | ||
assert_eq!(format!("{:.2e}", 1945), "1.94e3" ); | ||
assert_eq!(format!("{:.2e}", 1965), "1.96e3" ); | ||
assert_eq!(format!("{:.2e}", 1985), "1.98e3" ); |
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.
The more I look at this giant list of tests, the less I'm sure of its value. There are certainly some edge cases in here that we want to keep, but many are duplicating the exact same conditions. This makes it hard to see whether we actually have a test corresponding to the different pieces of the implementation.
For example, is there a test that exercises the power-of-10 overflow that we checked last time? I'm not sure.
Can we narrow it down to fewer targeted tests around different boundary conditions?
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.
Another possibility could be an exhaustive test comparing to the independent floating point implementation. Like, check all i16
values compared to the same f32
value, formatted to a few different levels of precision.
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.
for i in i16::MIN..=i16::MAX {
for p in 0..=5 {
assert_eq!(
format!("{i:.p$e}"),
format!("{:.p$e}", f32::from(i)),
"integer {i} at precision {p}"
);
}
}
4e4c972
to
3f0908f
Compare
☀️ Test successful - checks-actions |
Finished benchmarking commit (b917524): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 674.708s -> 674.219s (-0.07%) |
…-Simulacrum avoid exhaustive i16 test in Miri rust-lang#116301 added a test that is way too slow to be running in Miri. So let's only test a few hopefully representative cases.
Rollup merge of rust-lang#117946 - RalfJung:miri-libcore-test, r=Mark-Simulacrum avoid exhaustive i16 test in Miri rust-lang#116301 added a test that is way too slow to be running in Miri. So let's only test a few hopefully representative cases.
Pkgsrc changes: * Adapt checksums and patches. Upstream chnages: Version 1.76.0 (2024-02-08) ========================== Language -------- - [Document Rust ABI compatibility between various types] (rust-lang/rust#115476) - [Also: guarantee that char and u32 are ABI-compatible] (rust-lang/rust#118032) - [Warn against ambiguous wide pointer comparisons] (rust-lang/rust#117758) Compiler -------- - [Lint pinned `#[must_use]` pointers (in particular, `Box<T>` where `T` is `#[must_use]`) in `unused_must_use`.] (rust-lang/rust#118054) - [Soundness fix: fix computing the offset of an unsized field in a packed struct] (rust-lang/rust#118540) - [Soundness fix: fix dynamic size/align computation logic for packed types with dyn Trait tail] (rust-lang/rust#118538) - [Add `$message_type` field to distinguish json diagnostic outputs] (rust-lang/rust#115691) - [Enable Rust to use the EHCont security feature of Windows] (rust-lang/rust#118013) - [Add tier 3 {x86_64,i686}-win7-windows-msvc targets] (rust-lang/rust#118150) - [Add tier 3 aarch64-apple-watchos target] (rust-lang/rust#119074) - [Add tier 3 arm64e-apple-ios & arm64e-apple-darwin targets] (rust-lang/rust#115526) Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support. Libraries --------- - [Add a column number to `dbg!()`] (rust-lang/rust#114962) - [Add `std::hash::{DefaultHasher, RandomState}` exports] (rust-lang/rust#115694) - [Fix rounding issue with exponents in fmt] (rust-lang/rust#116301) - [Add T: ?Sized to `RwLockReadGuard` and `RwLockWriteGuard`'s Debug impls.] (rust-lang/rust#117138) - [Windows: Allow `File::create` to work on hidden files] (rust-lang/rust#116438) Stabilized APIs --------------- - [`Arc::unwrap_or_clone`] (https://doc.rust-lang.org/stable/std/sync/struct.Arc.html#method.unwrap_or_clone) - [`Rc::unwrap_or_clone`] (https://doc.rust-lang.org/stable/std/rc/struct.Rc.html#method.unwrap_or_clone) - [`Result::inspect`] (https://doc.rust-lang.org/stable/std/result/enum.Result.html#method.inspect) - [`Result::inspect_err`] (https://doc.rust-lang.org/stable/std/result/enum.Result.html#method.inspect_err) - [`Option::inspect`] (https://doc.rust-lang.org/stable/std/option/enum.Option.html#method.inspect) - [`type_name_of_val`] (https://doc.rust-lang.org/stable/std/any/fn.type_name_of_val.html) - [`std::hash::{DefaultHasher, RandomState}`] (https://doc.rust-lang.org/stable/std/hash/index.html#structs) These were previously available only through `std::collections::hash_map`. - [`ptr::{from_ref, from_mut}`] (https://doc.rust-lang.org/stable/std/ptr/fn.from_ref.html) - [`ptr::addr_eq`](https://doc.rust-lang.org/stable/std/ptr/fn.addr_eq.html) Cargo ----- See [Cargo release notes] (https://github.com/rust-lang/cargo/blob/master/CHANGELOG.md#cargo-176-2024-02-08). Rustdoc ------- - [Don't merge cfg and doc(cfg) attributes for re-exports] (rust-lang/rust#113091) - [rustdoc: allow resizing the sidebar / hiding the top bar] (rust-lang/rust#115660) - [rustdoc-search: add support for traits and associated types] (rust-lang/rust#116085) - [rustdoc: Add highlighting for comments in items declaration] (rust-lang/rust#117869) Compatibility Notes ------------------- - [Add allow-by-default lint for unit bindings] (rust-lang/rust#112380) This is expected to be upgraded to a warning by default in a future Rust release. Some macros emit bindings with type `()` with user-provided spans, which means that this lint will warn for user code. - [Remove x86_64-sun-solaris target.] (rust-lang/rust#118091) - [Remove asmjs-unknown-emscripten target] (rust-lang/rust#117338) - [Report errors in jobserver inherited through environment variables] (rust-lang/rust#113730) This [may warn](rust-lang/rust#120515) on benign problems too. - [Update the minimum external LLVM to 16.] (rust-lang/rust#117947) - [Improve `print_tts`](rust-lang/rust#114571) This change can break some naive manual parsing of token trees in proc macro code which expect a particular structure after `.to_string()`, rather than just arbitrary Rust code. - [Make `IMPLIED_BOUNDS_ENTAILMENT` into a hard error from a lint] (rust-lang/rust#117984) - [Vec's allocation behavior was changed when collecting some iterators] (rust-lang/rust#110353) Allocation behavior is currently not specified, nevertheless changes can be surprising. See [`impl FromIterator for Vec`] (https://doc.rust-lang.org/nightly/std/vec/struct.Vec.html#impl-FromIterator%3CT%3E-for-Vec%3CT%3E) for more details. - [Properly reject `default` on free const items] (rust-lang/rust#117818)
fixes issue #115737 , where the decimal places are rounded incorrectly when formatting scientific notation