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

Fix Issue 20508 - std.math.pow(-infinity, y) does not return NaN for imaginary or complex results #7363

Closed
wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jan 24, 2020

In addition to the fix I changed the public unittests. The current one mostly showed that corner cases work but not the usual usage (which already heavily varies, depending on the signs and integrity of the operands). I removed the corner cases completely from public view, because they are allready listed in a table above the examples and IMHO this doublication isn't useful. (The tests itself are preserved though.)

There are several additional issues burried in this function (including powers of -infinity). I started to identify them and I'll eventually report and address them as separate issues.

@ghost ghost requested a review from ibuclaw as a code owner January 24, 2020 08:17
@dlang-bot
Copy link
Contributor

dlang-bot commented Jan 24, 2020

Thanks for your pull request and interest in making D better, @berni44! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
20508 normal std.math.pow(-infinity, y) does not return NaN for imaginary or complex results

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + phobos#7363"

@ghost
Copy link
Author

ghost commented Jan 24, 2020

One of the public unittests did not work on windows, I replaced it with an other test (for the public tests, I want to use == instead of isClose/approxEqual, as the conversion to writeln makes the result a little bit ugly else).

@ghost
Copy link
Author

ghost commented Jan 24, 2020

+WIP (win64 is good now, but I need to find something, that works on win32 too)

@ghost
Copy link
Author

ghost commented Jan 24, 2020

looks like this is issue 20451 again...

@ghost ghost requested a review from wilzbach as a code owner January 24, 2020 12:43
@ghost
Copy link
Author

ghost commented Jan 24, 2020

I commented the first unittest out for win32 and freebsd. This works, but now the style checker does not recognize anymore, that there is a public example... For me, good examples are an important thing that I do not want to give up, just because the implementation for win32 is overdoing in accuracy. I could reverse the two public examples. That would work, but again, the documentation would suffer.

So I commented out the style check for public unittests for std.math until either the stylechecker gets smarter or issue 20451 is fixed (or someone has a better idea ;-) ).

If the tests work now, it's not anymore WIP.

std/math.d Outdated
@@ -8281,27 +8292,80 @@ if (isFloatingPoint!(F) && isFloatingPoint!(G))
return impl(x, y);
}

// Due to issue 20451 this doesn't work on Win32 and FreeBSD. It would work
// using isClose, but the main purpose of this test is to provide a good example,
// not to test the pow function, and this would lead to a somewhat ugly example.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The standard library documentation should not mislead users into thinking that floating-point operations are perfectly precise when they're not. Using isClose, approxEqual, or feqrel is the honest and correct option, even if it is less aesthetically pleasing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if you are aware of the transformations on the unittests, that are done by ddoc. Every assert is replaced by a writeln. But the output of writeln isn't compared anymore by a computer (but by the human reading the example), so the problem does not exist there. The strange comparisons can only be seen, when the user looks into the source code. And there they are next to this comment explaining what's going on.

I actually changed the comment and the versioning a little bit, because there might be other OSs which fail. Now this unittest is only used by ddoc. I hope this is sort of a compromise for you.

Bernhard Seckinger added 2 commits February 8, 2020 13:59
this is the documentation for pow() with two floating point arguments,
where for the sake of having a good example, the arguments are
compared by == instead of isClose. But that fails on some 32bit
computers. So the unittest is versioned out for ddoc use only, but now
the style checker does not recognize anymore, that there is indeed a
public example.
/*
Of course, floating point numbers should be compared using isClose,
but the main purpose of this test is to provide a good example,
not to test the pow function, and this would lead to ugly examples like:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good example is one that is appropriate to imitate, not just look at. Users should not be comparing the results of this pow function using ==, so the examples shouldn't either.

As noted in the author's TODO, the exp2(y * log2(x)) method currently used is very imprecise in some parts of its domain, so this isn't just quibbling about the occasional single-bit rounding error. Using isClose, feqrel, or approxEqual serves the practical purpose of warning the user about this, in addition to enabling automatic verification of the example.

but the main purpose of this test is to provide a good example,
not to test the pow function, and this would lead to ugly examples like:

writeln(isClose(pow(1.5, 10.0), 57.6650390625)); // true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wouldn't add the true.

writeln(pow(1.5, 10.0)); // 57.6650390625

So I versioned this unittest out, because on some 32bit computers the tests
would fail with ==.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's exactly what the dscanner tests tries to prevent people from doing. This will lead to user confusion if they copy this code to user machines and it fails there. Either:

  • use is close
  • use version (X86_64) inside the test, so that it's obvious to the user what's going on.

@wilzbach
Copy link
Member

wilzbach commented Feb 8, 2020

So I commented out the style check for public unittests for std.math until either the stylechecker gets smarter

No, this behavior is on purpose and by design as examples shouldn't be written with version (Ddoc). There's no way for us to ensure they actually work.

@tsbockman
Copy link
Contributor

Here's an example of a particularly imprecise result from pow:

void main()
{
    import std.stdio, std.math;
    
    // Computed in arbitrary precision via WolframAlpha:
    const(real) expected = 1.50354040799963854183e1854L;
    writefln("expected: %.20g", expected);
    
    const(real) got = pow(0.9832834778410573217L, -253259.351392875174952L);
    writefln("got: %.20g", got);
    
    const(int) incorrectBits = real.mant_dig - feqrel(got, expected); // 16
    writeln("number of incorrect bits: ", incorrectBits);
}

As a user, if I see official examples for a floating-point math function using ==, I expect a maximum error of 1 or 2 bits. The above example's 16 bits of error is bad enough to leave 80-bit real with worse precision than I would naively expect from 64-bit double.

Now floating-point pow turns out to be a very difficult function to implement precisely (I've tried), so I'm not saying this to attack the implementation. But, the documentation should reflect the reality that this pow is only an approximation.

@RazvanN7
Copy link
Collaborator

Closing this in favor of: #7747

@RazvanN7 RazvanN7 closed this Jan 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants