-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
TST: rewrite doctest in a floating-point-comparison friendly way #16336
Conversation
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
👋 Thank you for your draft pull request! Do you know that you can use |
But would scipy/scipy#20569 also fixes this problem without us having to do anything here? |
docs/coordinates/matchsep.rst
Outdated
>>> matches = catalog[idx] | ||
>>> (matches.separation_3d(c) == d3d).all() | ||
>>> np.allclose(matches.separation_3d(c), d3d, rtol=2e-16, atol=3e-13) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is user-facing documentation, so having to import numpy
and specify rtol
and atol
is distracting. It would be much better to display what d3d
is when it is computed, then display matches.separation_3d(c)
and let the reader compare the results themselves. Then we could also let the +FLOAT_CMP
directive take care of any floating point rounding differences in a way that is invisible to the readers.
Ideally the entire section would be rewritten in a similar manner, but it might be best to make a minimal change here just to solve the immediate problems in CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable. I agree that invoking np.allclose
is distracting. I just pushed your suggestion !
8c110bd
to
4720100
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think the entire section should be reworked, but fixing CI problems is urgent and doing a minimal change can be good enough for that purpose. That being said, @mhvk is very responsive so we can afford to wait for his review. Having more than one review is especially important for user-facing documentation because readability for non-experts cannot be judged with automated tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go with this for now! Thanks, @neutrinoceros!
Thanks, all! |
…omparison friendly way
…336-on-v6.1.x Backport PR #16336 on branch v6.1.x (TST: rewrite doctest in a floating-point-comparison friendly way)
Description
Fixes #16319,
similar to #16329, but with stricter margins than the defaults.This test worked fine on every archs for at least 5 years, so maybe it's hinting at a real bug that should be inspected more closely, but I'm really not sure it's worth it.