8000 DEP: deprecate np.finfo(None) by dshintani404 · Pull Request #23011 · numpy/numpy · GitHub
[go: up one dir, main page]

Skip to content

DEP: deprecate np.finfo(None) #23011

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 5 commits into from
Jan 19, 2023
Merged

Conversation

dshintani404
Copy link
Contributor

See #14684

if dtype is None:
warnings.warn(
"dtype cannot be None. This behavior "
"will raise an error in the future.",
Copy link
Member

Choose a reason for hiding this comment

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

Thanks, the warning is indeed the correct path. Could you add a test as well? It can live in test_deprecations.py.

Please add (Deprecated in NumPy 1.25) or something similar to the message. Could also lead with finfo() dtype ... just to make it even easier to find where it is coming from.

The last thing is that both here and in the test, we try to add a comment:

# Deprecated in NumPy 1.25, 2022-01-16

Or similar. This makes it much easier to find and to act on in the future.

@seberg
Copy link
Member
seberg commented Jan 18, 2023

Thanks, this looks good! I forgot, but since it is a deprecation, would you be up for also adding a brief release note? There is an explanation on how in doc/release/upcoming_changes/README.rst. It can be very short, even just a single bullet point.

@seberg
Copy link
Member
seberg commented Jan 19, 2023

Ver minimal :). Thanks for all the follow-ups @dshintani404.

@seberg
Copy link
Member
seberg commented Jan 19, 2023

CircleCI is a bit confused right and mostly seem to have run through even, should be fine.

@tupui about the circleCI failure here: CircleCI is failing here due to the SciPy incompatibility on current main. The point being: It wasn't failing on main, it seems even though plotting breaks the "with warnings" run ignoring the error result hides issues bigger then the warnings?

@seberg seberg merged commit 0cfbd3c into numpy:main Jan 19, 2023
@seberg
Copy link
Member
seberg commented Jan 19, 2023

@dshintani404 if you feel like digging into C and much more broadly. I don't mind that this zoomed in on np.finfo, but it may make sense to generally deprecate np.dtype(None) (which would have implied the deprecation here).

@tupui
Copy link
Contributor
tupui commented Jan 19, 2023

CircleCI is a bit confused right and mostly seem to have run through even, should be fine.

@tupui about the circleCI failure here: CircleCI is failing here due to the SciPy incompatibility on current main. The point being: It wasn't failing on main, it seems even though plotting breaks the "with warnings" run ignoring the error result hides issues bigger then the warnings?

Super strange, you might be able to temporary use a nitpick exception until you have a proper fix.

@seberg
Copy link
Member
seberg commented Jan 19, 2023

@tupui to be clear, what makes me pause is that I think it should be failing CI currently on all PRs, not that it failed here. I.e. see here:
https://app.circleci.com/pipelines/github/numpy/numpy/17679/workflows/136e9449-fae8-4069-9292-b20c1aed7820/jobs/29727#:~:text=199-,200,-201

(around line 180 of the "build devdocs" step). There is an exception building, but CI is green.

@tupui
Copy link
Contributor
tupui commented Jan 19, 2023

Yes this is what we said in the other PR, because there is a OR, it's always returning green. My understanding from what Matti said, was that it is fine as you need to really have a look either way.

@mattip
Copy link
Member
mattip commented Jan 19, 2023

I think I did not realize what I was agreeing to. Previously, we had a non-nitpick build that would fail properly when the build did not successfully complete, and an nitpick build that would always succeed but would print the number of warnings emitted. Now that we have compbined them, we do not have a way of indicating that there is a failure generating documentation, and not just a failure to format the generated documentation properly.

@seberg
Copy link
Member
seberg commented Jan 19, 2023

Yeah, I expected that a "critical" failure would float up but admit that is my error. So, not sure that this won't hide too many issues.

@tupui
Copy link
Contributor
tupui commented Jan 20, 2023

Let me know if I should add the second build back. One other possibility would be to not clean before rebuilding. This would be faster and still break if there is really an issue with the doc no?

@seberg
Copy link
Member
seberg commented Jan 20, 2023

Yeah, maybe lets do that, that would be great! I guess you are right about the not-cleaning, although I am not sure it actually speeds things up much.

@InessaPawson
Copy link
Member

Hi-five on merging your first pull request to NumPy, @dshintani404! We hope you stick around! Your choices aren’t limited to writing code – you can review pull requests, help us stay on top of new and old issues, develop educational material, work on our website, add or improve graphic design, create marketing materials, translate website content, write grant proposals, and help with other fundraising initiatives. For more info, check out: https://numpy.org/contribute
Also, consider joining our mailing list. This is a great way to connect with other cool people in our community and be part of important conversations that affect the development of NumPy: https://mail.python.org/mailman/listinfo/numpy-discussion

@dshintani404
Copy link
Contributor Author

Thank you for your help!

@dshintani404 dshintani404 deleted the deprecate-finfo-none branch January 21, 2023 12:03
@dshintani404
Copy link
Contributor Author
dshintani404 commented Jan 26, 2023

@dshintani404 if you feel like digging into C and much more broadly. I don't mind that this zoomed in on np.finfo, but it may make sense to generally deprecate np.dtype(None) (which would have implied the deprecation here).

I read relevant codes in C and understood that a point to fix was probably around dtypemeta.c, but now I am completely stacked.
Could you give me some hints where to read if possible? @seberg

@seberg
Copy link
Member
seberg commented Jan 26, 2023

Thee relevant code is in descriptor.c in the arraydescr_new function, you took a turn towards a more complicated chunk of code there. The _convert_from_any() call seems to accept None (which is probably OK).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

5 participants
0