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

Math.Pow does not follow IEEE 754 for all inputs #7511

Closed
tannergooding opened this issue Feb 26, 2017 · 13 comments
Closed

Math.Pow does not follow IEEE 754 for all inputs #7511

tannergooding opened this issue Feb 26, 2017 · 13 comments
Labels
area-System.Runtime enhancement Product code improvement that does NOT require public API changes/additions help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@tannergooding
Copy link
Member

The IEEE 754 spec defines the following inputs as special:

For the pown function (integral exponents only):

  • pown (x, 0) is 1 for any x (even a zero, quiet NaN, or infinity)
  • pown (±0, n) is ±∞ and signals the divideByZero exception for odd integral n < 0
  • pown (±0, n) is +∞ and signals the divideByZero exception for even integral n < 0
  • pown (±0, n) is +0 for even integral n > 0
  • pown (±0, n) is ±0 for odd integral n > 0.

For the pow function (integral exponents get special treatment):

  • pow (x, ±0) is 1 for any x (even a zero, quiet NaN, or infinity)
  • pow (±0, y) is ±∞ and signals the divideByZero exception for y an odd integer < 0
  • pow (±0, −∞) is +∞ with no exception
  • pow (±0, +∞) is +0 with no exception
  • pow (±0, y) is +∞ and signals the divideByZero exception for finite y < 0 and not an odd integer
  • pow (±0, y) is ±0 for finite y > 0 an odd integer
  • pow (±0, y) is +0 for finite y > 0 and not an odd integer
  • pow (−1, ±∞) is 1 with no exception
  • pow (+1, y) is 1 for any y (even a quiet NaN)
  • pow (x, y) signals the invalid operation exception for finite x < 0 and finite non-integer y.

For the powr function (derived by considering only exp(y * log(x))):

  • powr (x, ±0) is 1 for finite x > 0
  • powr (±0, y) is +∞ and signals the divideByZero exception for finite y < 0
  • powr (±0, −∞) is +∞
  • powr (±0, y) is +0 for y > 0
  • powr (+1, y) is 1 for finite y
  • powr (x, y) signals the invalid operation exception for x < 0
  • powr (±0, ±0) signals the invalid operation exception
  • powr (+∞, ±0) signals the invalid operation exception
  • powr (+1, ±∞) signals the invalid operation exception
  • powr (x, qNaN) is qNaN for x ≥ 0
  • powr (qNaN, y) is qNaN.

However, the current implementation does not return the correct values for a number of these inputs (especially around the handling of NaN).

@danmoseley
Copy link
Member

@dcwuser are you interested in getting cc'd on issues of such type? you did work in CoreFX to improve our handling of math edge cases.

@dcwuser
Copy link
Contributor

dcwuser commented Feb 28, 2017

Thanks @danmosemsft, yes I am interested.

Again, here is the spec: https://www.csee.umbc.edu/~tsimo1/CMSC455/IEEE-754-2008.pdf. The special cases of pow are listed on page 44.

Spec Violations

The bug filer doesn't say which specific cases he believes applies and are violated, so I'm going to try to whittle it down to specifics. Presumably the pown and powr cases are irrelevant, since we don't have those functions. I'm going to interpret "signals the invalid operation exception" as "return NaN", since that's what we do elsewhere and the spec says "the default result of an operation that signals the invalid operation exception shall be a quiet NaN". And I'm going to interpret "signals the divideByZero exception" to mean nothing at all, since those cases actually specify a return value and again, that's what we do elsewhere. Given all that, here are the three cases we violate:

P: We say Pow(-1, ±∞) = NaN, spec says 1.
Q: We say Pow(NaN, 0) = NaN, spec says 1.
R: We say Pow(1, NaN) = NaN, spec says 1.

Considerations

P arises by regarding infinities as even integers, presumably because all sufficiently big representable FP numbers are even integers. Of course, the counter-argument is that it isn't true that all sufficiently big real numbers are even integers, and it's real numbers that we are trying to represent here, so we shouldn't elevate properties that arise from the vagaries of the FP representation into definitive truths. This counter-argument is made slightly weaker, though, by the fact that we do respect Pow(-0, −∞) = +∞, which also is only true if we regard infinities as even integers.

Q and R arise from the principal that, if f(x) has a known, constant value c for all x != NaN, we should return that value for x = NaN, too. The idea is that NaN means "this is some real number, I just wasn't able to figure out which one", so if it doesn't matter which real number x was, we should swallow the NaN. One counter-argument is theoretical: NaN doesn't always mean that; for example, the NaN we get from taking the square root of a negative number actually means that the result is complex, and f(x) for complex x may well behave differently than for real x. An even better counter-argument is practical: if we swallow NaN's, the user might never investigate the potentially problematic parts of his code that generated them. The Wikipedia article on NaN's (https://en.wikipedia.org/wiki/NaN#Function_definition) addresses this controversy:

There are differences of opinion about the proper definition for the result of a numeric function that receives a quiet NaN as input. One view is that the NaN should propagate to the output of the function in all cases to propagate the indication of an error. Another view, and the one taken by the ISO C99 and IEEE 754-2008 standards in general, is that if the function has multiple arguments and the output is uniquely determined by all the non-NaN inputs (including infinity), then that value should be the result.

Personally I favor the always-propogate-NaNs principal over the swallow-NaNs-when-possible principal, but I didn't sit on the spec committee.

Recomendations

As with issue #7510, the spec violations are real but are violations of debatable spec conventions rather than mathematical conventions or truths. If the fixes are easy, have no perf impact, and we have cycles, we should go ahead and fix them. Otherwise, we shouldn't worry too much about them.

I regard the argument for P as better than the argument for Q and R, so if we are only going to do a partial fix that's the one I would pick.

@gkhanna79
Copy link
Member

CC @AlexGhiondea

@karelz
Copy link
Member

karelz commented Mar 15, 2017

Only one area path label please (avoids double counting in reports).

@tannergooding
Copy link
Member Author

Sorry for the delayed response here, but I listed all three because of the 'special' considerations for each function.

We definitely don't implement pown (because it only allows integral exponents). However, we could technically be implementing either pow or powr, depending on whether or not we special-case integer exponents.

That being said, since the current Math.Pow implementation calls down to the CRT implementation, where the C11 spec indicates:

The pow functions compute x raised to the power y. A domain error occurs if x is finite and negative and y is finite and not an integer value. A range error may occur. A domain error may occur if x is zero and y is zero. A domain error or pole error may occur if x is zero and y is less than zero.

We are definitely "implementing" pow and not powr 😄

@AlexGhiondea
Copy link
Contributor

This is something we should add, but it is not required for 2.0

@tannergooding
Copy link
Member Author

@AlexGhiondea, I actually already have a pr out that fixes this. We had incorrect special handling on classlibnative.

@tannergooding tannergooding self-assigned this Mar 20, 2017
@tannergooding
Copy link
Member Author

Self-assigning since this is fixed by dotnet/coreclr#10295

@stephentoub
Copy link
Member

Test fixes in corefx are still needed. Tests are now disabled against this issue.

@karelz
Copy link
Member

karelz commented Mar 23, 2017

@stephentoub which issue are they disabled against? This one?

@tannergooding
Copy link
Member Author

tannergooding commented Mar 23, 2017

@karelz, yes, this one. The PR is here: dotnet/corefx#17419

I have my own PR which improves the test coverage of Math and MathF in general here: dotnet/corefx#17217

I have already updated it to match the Math.Pow and Math.Atan2 fixes made on the CoreCLR side.

After dotnet/corefx#17419 goes in, I can rebase and resolve the merge conflicts and should be able to get the tests re-enabled in short order.

@tannergooding
Copy link
Member Author

The PR to re-enable the tests is just awaiting final sign-off before it gets merged. All jobs are green and I independently validated that the jobs have been appropriately partitioned into new vs legacy tests so that they pass on netfx as well.

@tannergooding
Copy link
Member Author

Fixed with dotnet/corefx#17217

@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the Future milestone Jan 31, 2020
@tannergooding tannergooding removed their assignment May 26, 2020
@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime enhancement Product code improvement that does NOT require public API changes/additions help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

No branches or pull requests

8 participants