Skip to content

[MRG] TST Increases tolerance to match float32 resolution #14184

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

Merged
merged 3 commits into from
Jun 27, 2019

Conversation

thomasjpfan
Copy link
Member

Reference Issues/PRs

Fixes #13868
Fixes #13876

What does this implement/fix? Explain your changes.

Changes tol to np.finfo(np.float32).resolution

Any other comments?

I suspect that float64 found a better (and different) solution because it can resolve 1e-10 better than float32 can.

Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@thomasjpfan thomasjpfan changed the title [MRG] TST Increases tolerance to match float32 resolution [MRG] TST Decreases tolerance to match float32 resolution Jun 25, 2019
@thomasjpfan thomasjpfan changed the title [MRG] TST Decreases tolerance to match float32 resolution [MRG] TST Increases tolerance to match float32 resolution Jun 25, 2019
@jnothman
Copy link
Member

jnothman commented Jun 26, 2019 via email

@thomasjpfan
Copy link
Member Author

For this PR, I was thinking that the different between two 32 bit floats can only be resolve up to np.finfo(np.float32).resolution=1e-6 precisely, while 64 floats can resolve up to np.finfo(np.float64).resolution=1e-15. With the original tol=1e-10, float64 can resolve differences in that range, and thus continue to find a better solution, while float32 would not be able to resolve. (This is all a hypothesis, I can not reproduce the error on my local machine with the dependencies from the CI).

Looking at the docs for sparse.linalg.cg the tol is treated like a rtol, which means the "correct" tol would depend on the norm of y.

@jnothman
Copy link
Member

jnothman commented Jun 26, 2019 via email

@thomasjpfan
Copy link
Member Author

thomasjpfan commented Jun 26, 2019

I’m going to see if this changes things on the CI for macPython by specifically pulling this branch.

@thomasjpfan
Copy link
Member Author

This pass on travis ci on osx.

Here is the diff of the scikit-learn-wheels version that was used to run the travis ci instances.

@jnothman jnothman added this to the 0.21.3 milestone Jun 27, 2019
@jnothman jnothman merged commit d360ffa into scikit-learn:master Jun 27, 2019
@jnothman
Copy link
Member

Thanks @thomasjpfan

koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
jnothman pushed a commit to jnothman/scikit-learn that referenced this pull request Jul 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ridge's test_dtype_match[sparse_cg] can fail on macOS
3 participants