rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.29k stars 12.72k forks source link

Float `signum` returns +/-1 for zeros, mismatching integers (and convention) #57543

Open huonw opened 5 years ago

huonw commented 5 years ago

The sign/signum function is almost always defined to have signum(x) = 0 if x = 0, e.g.

Currently, f32::signum and f64::signum do not satisfy this:

https://github.com/rust-lang/rust/blob/79d8a0fcefa5134db2a94739b1d18daa01fc6e9f/src/libstd/f32.rs#L1219-L1220

https://github.com/rust-lang/rust/blob/79d8a0fcefa5134db2a94739b1d18daa01fc6e9f/src/libstd/f64.rs#L1167-L1168

The sign of the signed zeroes can still be reflected when returning zero: (-0.0).signum() could return -0.0 like the Java APIs.

varkor commented 5 years ago

Agree that this is very counter-intuitive. Calling out the difference in behaviour for integers specifically, I think everyone would expect a test like the following to trivially pass.

let x = 0i8;
assert_eq!(x.signum() as f64, (x as f64).signum()); // ERROR
scottmcm commented 5 years ago

This sounds reasonable to me, especially with signum(-0.0) => -0.0. (I couldn't see any guidance for this in 754 -- copySign and isSignMinus are clearly different things unhelpful for deciding this.)

Is this change allowed, though? Does it need to be signum_better?

ExpHP commented 5 years ago

Is this change allowed, though? Does it need to be signum_better?

Surely nobody is considering changing the semantics of the existing function, I hope! I'm quite certain that even I have code that would break if this was changed. Simple things like dividing by signum. (or dividing a by b, where b happens to have a factor of signum(a)).

The current definition is not without its use cases. Namely, it treats +/-0. as limits of x approaching 0. (kind of like how sin(-0.0) produces -0.0, so that csc(-0.0) = sin(-0.0).recip() = -inf)

workingjubilee commented 4 years ago

@rustbot modify labels: +A-floating-point

workingjubilee commented 4 years ago

@ExpHP Dividing a negative number by {f32, f64}::signum is functionally equivalent to abs(a), and Rust's stability guarantee is not infinite leverage against fixing implementations, the question is whether or not this would be a fix.

However, I am not unsympathetic to your position, and in fact speaking of the absolute value function, I would say at the very least, making this return 0.0 always whether 0.0 or -0.0 is the input would be strictly incorrect, because it is symmetric with the absolute value function:

  1. The absolute value function returns the value of the number regardless of sign.
  2. The sign function returns the sign regardless of the value of the number.

As we have signed zeros in floating point, making signum(-0.0) equivalent to abs(-0.0) would break that symmetry. So any change would have to be signum(-0.0) => -0.0, which is what was suggested as a change.

And I should note, the signum function was actually adjusted slightly in Java. It now returns -0.0 if -0.0 is the input: https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/lang/Math.html#signum(double)

Importantly, however, regardless of whether the "zero is zero" implementation is chosen or not, a given number N would still be equivalent to abs(N) * signum(N), ignoring the vagaries of rounding in floating point. Of course, that is also true if signum zero returns 0 or 1 or -1 for integers, which... shrug!

EDIT: no, not shrug, that's kind of important, actually. That's why the value can be found described as undefined in math textbooks, so arguably it should be Option(N). It is useful however for it to "be 0" in many cases for various equations, by convention, but that's not how floats work since we do have signs on floats.

thomcc commented 4 years ago

I think ideally this would return +/-0.0 for +/-0.0 as input, but it's very obviously a breaking change. (And I don't really think it's a bug so much as a dubious API design)

Kimundi commented 3 years ago

Well we could deprecate it and introduce two new, better-named functions, one with the +1.0, +0.0, -0.0, -1.0 semantic, and one with the +1.0, -1.0 semantic. Would not be the first time that we have shuffled method names around like that in the std lib.

sollyucko commented 3 years ago

There could also be cases where it's useful to have an enum with the possible options.

thomcc commented 3 years ago

Well we could deprecate it and introduce two new, better-named functions

Yeah, it's complicated by the fact that the integer version has no reason to be deprecated, but perhaps this doesn't actually matter.

There could also be cases where it's useful to have an enum with the possible options.

This is already roughly PartialCmp with zero, no?

sollyucko commented 3 years ago

This is already roughly PartialCmp with zero, no?

Yeah, that's a good point. The only case this doesn't handle is if you need to distinguish between positive and negative zero.

Tachytaenius commented 1 year ago

Could call the mathematical one signum and rename the old one to signum2. Or sign and sign2 to make old projects not compile until fixed and for less ambiguous differentiation.

I feel the 2 is appropriate since atan2 doesn't make absolutely perfect sense either.