Skip to content
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

Support numpy >= 2.x on Python >= 3.8 #101

Merged
merged 4 commits into from
Oct 16, 2024

Conversation

manthey
Copy link
Contributor

@manthey manthey commented Oct 9, 2024

This switches from np.prod to math.prod when math.prod exists (Python >= 3.8). With this change, the tests all pass with numpy >= 2. Note that numpy 2 doesn't exist for Python < 3.8, so this is a safe check.

Closes #100.

@manthey manthey mentioned this pull request Oct 9, 2024
@pieper pieper requested review from hackermd and cgorman October 9, 2024 16:54
@manthey
Copy link
Contributor Author

manthey commented Oct 15, 2024

@pieper Is there anything I can do to help get this merged? Thank you.

@pieper
Copy link
Member

pieper commented Oct 15, 2024

I was hoping that someone with more a vested interest in this library might chime in to comment.

To me the goal sounds good, although I'd tend to think we should drop support for Python 2 at this point but I guess there's no point in potentially breaking things arbitrarily for a small fix.

Regarding the code itself, I'd suggest not repeating the check but instead centralizing it as a self._prod and putting the check in the init method.

@manthey
Copy link
Contributor Author

manthey commented Oct 15, 2024

To me the goal sounds good, although I'd tend to think we should drop support for Python 2 at this point but I guess there's no point in potentially breaking things arbitrarily for a small fix.

This would be dropping Python 3.6 and 3.7 (not 2.x), but that is a larger scope and I don't if there are any stakeholders of the library that still want retired Python versions.

Regarding the code itself, I'd suggest not repeating the check but instead centralizing it as a self._prod and putting the check in the init method.

Refactored in 0049c69

@pieper
Copy link
Member

pieper commented Oct 15, 2024

So the 3.7 test is failing, which I guess is expected? Should we somehow force older numpy for 3.7 or drop it?

https://github.com/ImagingDataCommons/dicomweb-client/actions/runs/11353082712/job/31582437962?pr=101

This needs to be done before we try to compute a prod.
@manthey
Copy link
Contributor Author

manthey commented Oct 16, 2024

So the 3.7 test is failing, which I guess is expected? Should we somehow force older numpy for 3.7 or drop it?

https://github.com/ImagingDataCommons/dicomweb-client/actions/runs/11353082712/job/31582437962?pr=101

I should have placed the code that checks for math.prod earlier in the __init__ method. See 1076427.

@pieper
Copy link
Member

pieper commented Oct 16, 2024

@pieper
Copy link
Member

pieper commented Oct 16, 2024

Okay, looks like that got it 👍

@pieper pieper self-requested a review October 16, 2024 14:23
Copy link
Member

@pieper pieper left a comment

Choose a reason for hiding this comment

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

Thanks for working through this.

@pieper pieper merged commit 10a2538 into ImagingDataCommons:master Oct 16, 2024
4 checks passed
@manthey
Copy link
Contributor Author

manthey commented Oct 16, 2024

Thank you!

@manthey
Copy link
Contributor Author

manthey commented Oct 16, 2024

@pieper And, lastly, can we do a patch version release? It looks like the process is to modify the version in https://github.com/ImagingDataCommons/dicomweb-client/blob/master/src/dicomweb_client/__init__.py and then create a github release. I can make a PR for the version number, if that is useful.

@manthey manthey deleted the numpy-2 branch October 16, 2024 15:04
@pieper
Copy link
Member

pieper commented Oct 16, 2024

Yes, if you don't mind making a PR that would be great.

I don't think I've ever making a release here or getting it onto pypi. But I see that @CPBridge did the last one so maybe he can help here.

@CPBridge
Copy link
Collaborator

Yes I can do this. Sorry I have been a bit negligent on this repo. I see there are another couple of PRs that could be quick merges and then I'll push out a release. Thanks @manthey for your help on this!

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.

numpy 2 compatibility
3 participants