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 5628 - std.math unittest disabled - roundoff error in pow() #7321

Merged
merged 1 commit into from Dec 25, 2019
Merged

Fix Issue 5628 - std.math unittest disabled - roundoff error in pow() #7321

merged 1 commit into from Dec 25, 2019

Conversation

ghost
Copy link

@ghost ghost commented Dec 15, 2019

The original tests compare apples and oranges, that is, left side double/float, right side real. The values on the left side are correct in the precision available but real has better precision and therefore the tests failed. Casting to the lower precision fixes this.

As a side note: I dislike these tests, because the right side could still produce wrong results, depending on the implementation of * and / and rounding errors. I'd prefere to compare to manually calculated literals.

@ghost ghost requested a review from ibuclaw as a code owner December 15, 2019 10:40
@dlang-bot
Copy link
Contributor

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
5628 normal std.math unittest disabled - roundoff error in pow() on SSE2

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#7321"

@ghost
Copy link
Author

ghost commented Dec 15, 2019

WIP: The unittest failed on FreeBSD. i added some writelns to see, what happens...

@wilzbach
Copy link
Member

WIP: The unittest failed on FreeBSD. i added some writelns to see, what happens...

FYI: We only semi-officially support FreeBSD. For example, std.net.curl isn't supported at all anymore and with the upcoming FreeBSD 12 isn't supported either. So if there's no immediate fix/solution, I would suggest not to bother about FreeBSD (i.e. disable it for FreeBSD) and waste time into that sink, but into more interesting and impactful things ;-)

@ghost
Copy link
Author

ghost commented Dec 15, 2019

Unfortunately win32 has the same bug.

@ghost
Copy link
Author

ghost commented Dec 15, 2019

Not WIP anymore. I found a workaround with approxEqual. The constants used there are so small, that it's actually comparing for equality.

IMHO it's a bug in DMD and I filed a bugreport. We are discussing it there currently.

@kinke
Copy link
Contributor

kinke commented Dec 20, 2019

I don't think the compiler is to blame here at all. The problem/surprising fact is that this templated pow overload takes a F floating-point input type, but always uses real p = 1.0, v = void; - that's what's causing the excess/surprising intermediate precision.

The original tests compare apples and oranges

Absolutely.

@ghost
Copy link
Author

ghost commented Dec 21, 2019

I don't think the compiler is to blame here at all. The problem/surprising fact is that this templated pow overload takes a F floating-point input type, but always uses real p = 1.0, v = void; - that's what's causing the excess/surprising intermediate precision.

From the discussions in the bugreport I meanwhile know, that "intermediate results" is stretched far more on 32bit machines than I thought. The result type of pow is essentially F, but the compiler returns a real. If that would be cast to F before returning, as I think it should, it would work.

@thewilsonator
Copy link
Contributor

This is green, is it good to go?

@ghost
Copy link
Author

ghost commented Dec 25, 2019

IMHO yes

@dlang-bot dlang-bot merged commit 76e5694 into dlang:master Dec 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants