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.

Sign up for GitHub

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

Jump to bottom

builtins::math::tests::expm1 MacOS Fix #664

Closed
neeldug opened this issue Aug 26, 2020 · 3 comments · Fixed by #665
Closed

builtins::math::tests::expm1 MacOS Fix #664

neeldug opened this issue Aug 26, 2020 · 3 comments · Fixed by #665
Assignees
@neeldug
Labels
bug Something isn't working test Issues and PRs related to the tests.
Milestone
v0.10.0

Comments

@neeldug
Copy link
Contributor

neeldug commented Aug 26, 2020

Describe the bug
This is a bug which is inherent throughout all languages due to the non-trivial floating point arithmetic varying implementations across platforms, this will result in certain platforms failing tests for various float-based operations that are non-trivial.

To Reproduce
Run cargo test on MacOS, and probably a few other OS's.

Expected behavior
All tests should pass, as these failures are simply due to floating point inaccuracies.

Build environment (please complete the following information):

  • OS: MacOS
  • Version: 10.15.6
  • Target triple: Darwin
  • Rustc version: rustc 1.45.2 (d3fb005a3 2020-07-31)

Additional context
This should be solvable with use of a tolerance or by using some approximately equal assertion, just quickly looking over these, approx or float-cmp, the latter seems much more often used, this may also be used in other cases where tests fail in future cases for floating point arithmetic.
@neeldug neeldug added the bug Something isn't working label Aug 26, 2020
@neeldug
Copy link
Contributor Author

neeldug commented Aug 26, 2020

I am planning to write a PR, using float-cmp for this, however, I'll wait for the go-ahead!

@Razican
Copy link
Member

Razican commented Aug 26, 2020

Go ahead!

@neeldug neeldug mentioned this issue Aug 26, 2020
Merged
@neeldug
Copy link
Contributor Author

neeldug commented Aug 26, 2020 •
edited
Loading

In fact, with float-cmp we can reinstate the builtins::math::tests::tan test, as this was commented due to a similar issue, this is linked to #260 where the test had to be commented out. I think perhaps it may be valuable to refactor many float-based tests in the math module to using float-cmp for assertions that they pass.

Specifically for refactoring the currently commented test for tan, should I submit this in a separate PR branched from this PR?

@Razican Razican added the test Issues and PRs related to the tests. label Aug 27, 2020
@Razican Razican added this to the v0.10.0 milestone Aug 27, 2020
Razican pushed a commit that referenced this issue Aug 28, 2020
@neeldug
Added approx_eq! macro for expm1 tests. (#665)
2d31ea3 
* Added approx_eq! macro for expm1 tests.

Added approx_eq! macro for expm1 tests due to floating point arithmetic
inaccuracies, using default ULP & epsilon values. approx_eq! macro does
not work for NaN values, however, for tests this should be okay anyway!
Solves #664.

* Modified Cargo.toml to remove unneeded feature.

Removed std feature as was unused in test.

* Moves `float-cmp` to Dev Dependencies

Refactors tan() unit test, previously unused, to use approx_eq!() macro
for assertion to pass on CI.

* Fixes cargo fmt errors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@neeldug neeldug

Labels
bug Something isn't working test Issues and PRs related to the tests.
Projects
None yet
Milestone
v0.10.0
Development

Successfully merging a pull request may close this issue.

Added approx_eq! macro for expm1 tests.
2 participants
@Razican @neeldug