Skip to content
This repository was archived by the owner on Apr 28, 2025. It is now read-only.

rem_pio2: actually return medium value for x ~ 2pi/2 #165

Merged
merged 3 commits into from
May 13, 2019

Conversation

m1el
Copy link
Contributor

@m1el m1el commented May 13, 2019

There's a discrepancy between musl's rem_pio2 and libm's rem_pio2:
The calculation result for medium case is not used for x ~ pi/2..2pi/2

@m1el
Copy link
Contributor Author

m1el commented May 13, 2019

Adding the #[must_use] attribute to scoped medium function would ensure that all calls are meaningful, but I think it's not worth doing.

@alexcrichton
Copy link
Member

Thanks! Can a test be added for this since it presumably wasn't caught by the existing tests?

@m1el
Copy link
Contributor Author

m1el commented May 13, 2019

I'll try to find a test case for this, but it might be the case that this is an optimization for this specific range.

Although it should be obvious from the code that currently the return value of medium is discarded, and control flow doesn't follow musl's implementation.

@m1el
Copy link
Contributor Author

m1el commented May 13, 2019

Added tests for this change, public interface sin(f64) is affected.

@m1el
Copy link
Contributor Author

m1el commented May 13, 2019

The reason general tests didn't catch it: this difference only affects some values in range [0x400921fb00000000..0x400921fbffffffff] or [3.141592025756836..3.1415939331054683], which has a rate of ~1e-10.

@alexcrichton
Copy link
Member

@bors: r+

Hm ok, perhaps the random generation for floats could be updated to have a low-probability path to generate values near pi and/or multiples of pi?

@alexcrichton alexcrichton merged commit de8ed2d into rust-lang:master May 13, 2019
@m1el
Copy link
Contributor Author

m1el commented May 13, 2019

@alexcrichton RNG that generates "interesting" numbers more often is definitely doable, that's a great idea. It should probably be implemented under a different issue/PR.

@Schultzer Schultzer mentioned this pull request Jul 2, 2019
tgross35 pushed a commit that referenced this pull request Apr 18, 2025
rem_pio2: actually return medium value for x ~ 2pi/2
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants