Skip to content
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

{f32,f64}::DIGITS is misleading #89106

Open
workingjubilee opened this issue Sep 19, 2021 · 6 comments
Open

{f32,f64}::DIGITS is misleading #89106

workingjubilee opened this issue Sep 19, 2021 · 6 comments
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools A-floating-point Area: Floating point numbers and arithmetic

Comments

@workingjubilee
Copy link
Member

workingjubilee commented Sep 19, 2021

While they come with a caveat that they are an "approximate" value, these constants are treated as gospel and invoked by clippy in the excessive_precision lint to create a warning. Clippy is wrong, however, in doing so. The numbers currently embedded in these constants reflect the lower bound, beyond which precision cannot be certain.

A binary32 may, however, contain up to 9 significant decimal digits, a binary64 may contain up to 17 decimal digits, and it is these numbers that are recommended as minimums for printing decimal strings if one wants to assure it will be parsed back as the actual value. In other words, there is a range. These const values have lead people to misleading conclusions, the correct answer to which cannot even be inferred from the documentation since there is no specification on how they are "approximate".

Either these constants should reflect the upper bound of precision, another constant should be embedded alongside them reflecting the other side of the bound and the documentation should discuss both ends of the range, or they should be deprecated wholesale.

EDIT: I misunderstood a segment of the code and Clippy's is a touch less problematic than I thought. Alas, precisely because of even more confusing naming from confusing names...

@workingjubilee workingjubilee added A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools A-floating-point Area: Floating point numbers and arithmetic labels Sep 19, 2021
@mjclements
Copy link

This is interesting. To give a concrete example, the print out in the

fn main() {
    let a: f32 = 0.111294314;
    let b: f32 = 0.111294307;
    println!("with precision 6; does {0:.6} equal {1:.6}?", a, b);
    println!("with precision 7; does {0:.7} equal {1:.7}?", a, b);
    println!("with precision 8; does {0:.8} equal {1:.8}?", a, b);
    println!("with precision 9; does {0:.9} equal {1:.9}?", a, b);
    assert_eq!(a, b);
}

output:
with precision 6; does 0.111294 equal 0.111294?
with precision 7; does 0.1112943 equal 0.1112943?
with precision 8; does 0.11129431 equal 0.11129431?
with precision 9; does 0.111294314 equal 0.111294307?

thread 'main' panicked at 'assertion failed: (left == right)
left: 0.111294314,
right: 0.11129431', src/main.rs:8:1

That's just one example of a pair of adjacent f32s that needs 9 digits of precision to differentiate. They do seem to be on the rarer side. I would kinda support changing the DIGITS constant to the upper bound. If the current constants are left it, it might be kinda neat to tell users approximately what percentage of the time digits is too low.
(https://stackoverflow.com/questions/60790120/which-single-precision-floating-point-numbers-need-9-significant-decimal-digits for reference).

@CryZe
Copy link
Contributor

CryZe commented Sep 19, 2021

Similar issue #88734

@m-ou-se
Copy link
Member

m-ou-se commented Sep 19, 2021

This constant doesn't seem to be used a lot. Grepping through the latest version of all crates on crates.io for f(32|64)::DIGITS gives very few results:

sjq-0.1.4/src/parse_basics.rs
37:static FRACTIONAL_PART_MAX_LENGTH: Lazy<usize> = Lazy::new(|| std::f64::DIGITS as usize);

bigdecimal-0.2.0/src/lib.rs
1780:            PRECISION = ::std::f32::DIGITS as usize
1793:            PRECISION = ::std::f64::DIGITS as usize

datom-bigdecimal-0.3.1/src/lib.rs
1776:            PRECISION = ::std::f32::DIGITS as usize
1789:            PRECISION = ::std::f64::DIGITS as usize

gluon_vm-0.9.4/src/primitives.rs
305:            digits => f64::DIGITS,

core-nightly-2015.1.7/src/num/mod.rs
1286:    #[deprecated = "use `std::f32::DIGITS` or `std::f64::DIGITS` as appropriate"]

no_proto-0.9.6/src/pointer/dec.rs
466:    for digits in 0..core::f64::DIGITS {
479:    for digits in 0..core::f64::DIGITS {

half-1.7.1/src/binary16.rs
976:        assert_eq!(core::f32::DIGITS, digits32);

micromath-2.0.0-pre/src/float.rs
79:    pub const DIGITS: u32 = f32::DIGITS;

bigdecimal-rs-0.2.1/src/lib.rs
1776:            PRECISION = ::std::f32::DIGITS as usize
1789:            PRECISION = ::std::f64::DIGITS as usize

(Almost all of these use the deprecated one in {std,core}::{f32,f64}::, instead of the one associated to the type directly.)

We should probably just deprecate DIGITS since its meaning is unclear and confusing.

(Also why is f32::DIGITS = 6? Shouldn't it be at least 7?)

@m-ou-se
Copy link
Member

m-ou-se commented Sep 19, 2021

A few more that weren't matched by my regex:


smth-0.3.0/src/traits.rs
151:            const DIGITS: u32 = std::$ty::DIGITS;

funty-1.2.0/src/lib.rs
1402:			const DIGITS: u32 = core::$t::DIGITS;

tstr-0.2.1/src/to_uint.rs
102:    const DIGITS: u32 = T::DIGITS;

tstr-0.2.1/src/to_uint/impl_no_const_generics.rs
170:                    sum = $ty::U128 + sum * ten_pow($ty::DIGITS);
174:            const DIGITS: u32 = 0 $( + $ty::DIGITS )*;

@agurney
Copy link

agurney commented Sep 25, 2021

f32::DIGITS cannot be 7 since 8589973000.0 and 8589974000.0 round to the same f32 and are different at the 7th decimal digit.

@eddyb
Copy link
Member

eddyb commented Feb 23, 2022

The linked clippy code looks suspicious in at least one particular way: it doesn't literally parse both the original and the proposed replacement, into the appropriate type, to do a bitwise comparison.

It only parses the original and the only obviously-exact operation is value.fract() == 0.0 (the other use of value is formatting it to a string).

For a MachineApplicable lint, it should IMO guarantee the exact same bitwise result from recompiling after the change is applied, instead of implicitly trusting that no precision loss occurs along the way.

(Also it should parse with both rustc_apfloat and std, and assert bitwise equal results, like rustc itself does, if it wants to be maximally defensive, but that's less necessary since compilation itself checks that)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools A-floating-point Area: Floating point numbers and arithmetic
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants