Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
charris merged 10 commits intonumpy:mainfromTontonio3:isclose-inf-tol-war
May 19, 2025

Conversation

Tontonio3
Copy link
Contributor

Added warning messages if atol or rtol are numpy.nan or numpy.inf within is close

Closes issue#28205

Copy link
Member

@ngoldbaumngoldbaum left a 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.


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)

Copy link
Member

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.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks, will do

Copy link
Member

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.

@@ -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
Copy link
Member

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

Copy link
ContributorAuthor

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

@seberg
Copy link
Member

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":

  • Unless we change this a bit, I think we could just merge things into a singlenp.isfinite() call? You could still add the second step if an error occurs (trade-off to make the normal thing faster). Micro-optimizers might even be tempted to optimize the normal scalar case, but I don't want to worry about it.
  • This means we give a warning no matter the usernp.errstate() settings. I am slightly torn on that. OTOH, passing inf/nan is strange here. OTOH, if atol/rtol is an array for some reason(?) it might be reasonable to want to honornp.errstate().
  • Typically, if a value is alreadyNaN we don't give a warning for comparisons. But that is just a (partially historic from compiler bugs) numpy thing, IEEE actually wants it for<= and I guess it makes sense here.
    (another thing that could make some sense is making NaN before like 0 for rtol/atol?)

I was wondering if re-arranging/narrower scoping of theerrstate context helps. I suspect it might work, but only by replacing non-finite values inabs(y) with 0 or so, so not sure it is a good path. (something to explorer if we care about theerrstate honoring only!)

@Tontonio3
Copy link
ContributorAuthor

@seberg I will look into that, but interestingly at the start I did not use np.any, but it gave me errors without it

@ngoldbaum
Copy link
Member

This means we give a warning no matter the user np.errstate() settings. I am slightly torn on that. OTOH, passing inf/nan is strange here. OTOH, if atol/rtol is an array for some reason(?) it might be reasonable to want to honor np.errstate().

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.

@ngoldbaumngoldbaum added the 56 - Needs Release Note.Needs an entry in doc/release/upcoming_changes labelJan 30, 2025
@Tontonio3
Copy link
ContributorAuthor

  • This means we give a warning no matter the usernp.errstate() settings. I am slightly torn on that. OTOH, passing inf/nan is strange here. OTOH, if atol/rtol is an array for some reason(?) it might be reasonable to want to honornp.errstate().

There is an slight issue with this as the invalid errstate is always'ignore', so should I check the errstate before the with? I can do it, but I don't know

with errstate(invalid='ignore'):        # atol and rtol can be arrays        if not (np.all(np.isfinite(atol)) and np.all(np.isfinite(rtol))):            warnings.warn(f"One of rtol or atol is not valid, atol: {atol}, rtol: {rtol}", stacklevel=2)          result = (less_equal(abs(x - y), atol + rtol * abs(y))                  & isfinite(y)                  | (x == y))        if equal_nan:            result |= isnan(x) & isnan(y)```

@Tontonio3
Copy link
ContributorAuthor

@ngoldbaum Hey, I've implemented the suggested changes and have done the release note. Is there anything else that's required?

@ngoldbaumngoldbaum added this to the2.3.0 release milestoneMar 7, 2025
@ngoldbaum
Copy link
Member

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 withmain. I added this to the 2.3 milestone which should ensure another developer will look at this before the next release.

@Tontonio3
Copy link
ContributorAuthor

@ngoldbaum Thanks! The rebase worked

@charrischarris merged commitbcf0de0 intonumpy:mainMay 19, 2025
72 checks passed
@github-project-automationgithub-project-automationbot moved this fromAwaiting a code review toCompleted inNumPy first-time contributor PRsMay 19, 2025
@charris
Copy link
Member

Thanks@Tontonio3 .

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@ngoldbaumngoldbaumngoldbaum approved these changes

Assignees
No one assigned
Labels
00 - Bug56 - Needs Release Note.Needs an entry in doc/release/upcoming_changes
Projects
Milestone
2.3.0 release
Development

Successfully merging this pull request may close these issues.

4 participants
@Tontonio3@seberg@ngoldbaum@charris

[8]ページ先頭

©2009-2025 Movatter.jp