Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork10.9k
BUG: allclose does not warn for invalid value encountered in multiply or alternativly checks for validity of tolerances#28221
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
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.
Thanks! This is definitely an improvement and could be merged as-is but I think there are some pretty obvious improvements that will hopefully be easy for you to take care of.
numpy/_core/numeric.py Outdated
if np.any(np.isnan(atol)) or np.any(np.isnan(rtol)): | ||
warnings.warn("At least one of rtol and atol are not a number", stacklevel=2) | ||
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.
If you use f-strings you can pretty easily make these warnings nicer, e.g. actually print out which one was nan or inf.
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.
Thanks, will do
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 like to checkgit diff main
locally to fix stuff like this.
Uh oh!
There was an error while loading.Please reload this page.
numpy/lib/tests/test_loadtxt.py Outdated
@@ -1072,4 +1072,4 @@ def test_maxrows_exceeding_chunksize(nmax): | |||
fh.write("\n".join(data)) | |||
res = np.loadtxt(fname, dtype=str, delimiter=" ", max_rows=nmax) | |||
os.remove(fname) | |||
assert len(res) == nmax | |||
assert len(res) == nmax |
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.
total nitpick but it’s best practice not to include unrelated changes like this in open source pull requests
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 made a couple mistakes with my git, so that's a product of that
I had some thoughts, I'll just post. I am sorry, I hadn't quite finished them through, so the likely answer to most is "no need to worry":
I was wondering if re-arranging/narrower scoping of the |
@seberg I will look into that, but interestingly at the start I did not use np.any, but it gave me errors without it |
Seems reasonable to check the errstate before doing the warning, and e.g. ignore it or turn it into an error depending on the setting. I agree this is an extra check in the fast path in service ofmaybe generating a warning in an uncommon case. That said, in almost all cases rtol and atol are scalars so this won't matter. If rtol and atol are arrays then if we check the errstate we can still give an escape hatch to bypass the isnan calls. Also on reflection this is a behavior change and needs a release note. |
There is an slight issue with this as the invalid errstate is always
|
@ngoldbaum Hey, I've implemented the suggested changes and have done the release note. Is there anything else that's required? |
Sorry, I dropped this. I think the test failure will go away if you rebase on or merge with |
d654312
tob0b40f4
Compare@ngoldbaum Thanks! The rebase worked |
bcf0de0
intonumpy:mainUh oh!
There was an error while loading.Please reload this page.
Thanks@Tontonio3 . |
Added warning messages if atol or rtol are numpy.nan or numpy.inf within is close
Closes issue#28205