Skip to content

BUG: fix compatibility with numpy 1.25 (future)#333

Merged
neutrinoceros merged 1 commit intoyt-project:mainfrom
neutrinoceros:reg_numpy_22744
Dec 7, 2022
Merged

BUG: fix compatibility with numpy 1.25 (future)#333
neutrinoceros merged 1 commit intoyt-project:mainfrom
neutrinoceros:reg_numpy_22744

Conversation

@neutrinoceros
Copy link
Copy Markdown
Member

upstream test case from yt, see yt-project/yt#4241
For further context, see numpy/numpy#22744

@neutrinoceros
Copy link
Copy Markdown
Member Author

@ngoldbaum I'll try to have a look at fixing the problem in unyt, as is being suggested in numpy/numpy#22744, but this PR can be merged as is so we can capture the regression on unyt's CI rather than yt's

@ngoldbaum
Copy link
Copy Markdown
Contributor

I'm looking at fixing the bug right now actually. My plan was to push to this PR and merge it with the fix.

@neutrinoceros
Copy link
Copy Markdown
Member Author

fine by me ! it's actually getting pretty late in my time zone so, by all means !

@neutrinoceros
Copy link
Copy Markdown
Member Author

Also, given that it's known to break downstream code (yt), and we have no release date for unyt 3.0, I think it might be worth doing a bugfix release with just this fix, if it's small enough

@ngoldbaum
Copy link
Copy Markdown
Contributor

Right now we don't have any kind of maintenance branch in the repo. We also haven't done a release since we switched to pyproject.toml, so we need to be careful to make sure the sdist and wheel we upload have all the necessary metadata. If I give you upload rights on pypi would you be willing to handle the necessary git and python distribution stuff and release unyt 2.9.3? If so give me your pypi username so I can add you. Please also feel free to include any other backports you'd like to include, and don't forget to update the changelog.

@neutrinoceros
Copy link
Copy Markdown
Member Author

I'm game to do this !
What I had in mind was to branch out from the 2.9.2 tag. I'll manually go over the merge history to see if there's anything else worth backporting. My PyPI username is crobert

@neutrinoceros
Copy link
Copy Markdown
Member Author

neutrinoceros commented Dec 6, 2022

Final note from me today but this is not going the way I had in mind; my understanding was that comparing a unyt quantity to a string should be possible (the result would always be 0).

That's still how numpy 0D arrays behave and that's the expectation from yt's standpoint. If we're going to embrace it as invalid then it means yt needs a fix too.

@ngoldbaum
Copy link
Copy Markdown
Contributor

What I ended up settling on is to follow numpy's future behavior and return an array filled with bools if comparison fails.

@neutrinoceros
Copy link
Copy Markdown
Member Author

Awesome, thank you very much !
About doing the release, is it safe for me to use test.pypi or do I need an other invite there ? (I'm not sure how frequently this index is purged, but it looks like unyt is currently not there)
just in case, my username there is neutrinoceros

Copy link
Copy Markdown
Member Author

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

I have a couple suggestions, but overall this is great.

@neutrinoceros neutrinoceros changed the title TST: add a test to cover numpy regression BUG: fix compatibility with numpy 1.25 (future) Dec 7, 2022
@neutrinoceros neutrinoceros added the bug Something isn't working label Dec 7, 2022
Co-authored-by: Nathan Goldbaum Nathan Goldbaum <nathan12343@gmail.com>
@neutrinoceros neutrinoceros merged commit fdc8037 into yt-project:main Dec 7, 2022
@neutrinoceros neutrinoceros deleted the reg_numpy_22744 branch December 7, 2022 10:07
neutrinoceros added a commit to neutrinoceros/unyt that referenced this pull request Dec 7, 2022
BUG: fix compatibility with numpy 1.25 (future)
neutrinoceros added a commit to neutrinoceros/unyt that referenced this pull request Dec 7, 2022
BUG: fix compatibility with numpy 1.25 (future)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants