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

Fix compat with Numpy 1.24 #14193

Merged
merged 3 commits into from Dec 19, 2022
Merged

Fix compat with Numpy 1.24 #14193

merged 3 commits into from Dec 19, 2022

Conversation

saimn
Copy link
Contributor

@saimn saimn commented Dec 19, 2022

Fix #14189

The compat code added in #14128 assumed the keepdims change (numpy/numpy#22721) would concern Numpy 1.25, but this change made it to Numpy 1.24 (numpy/numpy#22748).

The compat code added in astropy#14128 assumed the keepdims change would
concern Numpy 1.25, but this change made it to Numpy 1.24.
@saimn saimn added no-changelog-entry-needed 💤 backport-v5.0.x on-merge: backport to v5.0.x 💤 backport-v5.2.x on-merge: backport to v5.2.x labels Dec 19, 2022
@saimn saimn added this to the v5.0.6 milestone Dec 19, 2022
@saimn saimn requested a review from mhvk December 19, 2022 17:16
Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

Ah, that would make sense! Approving, though obviously we need to check the tests!

@mhvk
Copy link
Contributor

mhvk commented Dec 19, 2022

Yes, it works! (At least on the first test).

Aside, now that 1.24 is released, we probably should change utils/compat/numpycompat to 1.24dev0 to 1.24.

@@ -587,7 +587,7 @@ def median(a, axis=None, out=None, **kwargs):

a = Masked(a)

if NUMPY_LT_1_25:
if NUMPY_LT_1_24:
Copy link
Member

Choose a reason for hiding this comment

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

Hmm... What does this mean? An upstream patch that made it into 1.24rc was backed out last minute?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, no, wait... a patch that was not in 1.24rc got backported from 1.25.dev last minute?

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, I think so. Bit weird, but I'm not exactly sure where the were in the release schedule when I made my PR.

@pllim pllim added the Extra CI Run cron CI as part of PR label Dec 19, 2022
@pllim
Copy link
Member

pllim commented Dec 19, 2022

I also want to see if this will fix the cron jobs. Thanks!

@pllim
Copy link
Member

pllim commented Dec 19, 2022

@astrofrog , probably want a 5.2.1 soon with this patch?

@@ -1696,27 +1696,32 @@ def itemset(self, *args):
self.view(np.ndarray).itemset(*(args[:-1] + (self._to_own_unit(args[-1]),)))

def tostring(self, order="C"):
"""Not implemented, use ``.value.tostring()`` instead."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, it seems like there has been a change on how docstrings are inherited. How weird. Anyway, happy with this if it works!

Copy link
Contributor

Choose a reason for hiding this comment

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

And not just happy if thjs works: this is actually an improvement, since now if someone does quantity.tostring? in ipython, they get a useful docstring!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem was a Sphinx reference error with frombuffer from the docstring inherited from ndarray.tobytes :

❯ pydoc numpy.ndarray.tobytes 
...    
    See also
    --------
    frombuffer
        Inverse of this operation, construct a 1-dimensional array from Python
        bytes.

So adding our own docstring was the easy solution, and while doing that I tried to put a docstring that could be useful ;)

@pllim
Copy link
Member

pllim commented Dec 19, 2022

Link check failure is unrelated. I think I am satisfied if one of the two exotic arch jobs pass and I can cancel the other one since the last one always takes so long. Just want to make sure the stuff we using for the CI works.

@mhvk
Copy link
Contributor

mhvk commented Dec 19, 2022

The error in the link check is:

2022-12-19T19:04:03.9135598Z Warning, treated as error:
2022-12-19T19:04:03.9136308Z /home/runner/work/astropy/astropy/docs/whatsnew/5.2.rst:3:broken link: https://docs.astropy.org/en/v5.2.x/whatsnew/5.2.html (404 Client Error: Not Found for url: https://docs.astropy.org/en/v5.2.x/whatsnew/5.2.html)

There is indeed no v5.2.x on readthedocs; I think that should be latest or 5.2?

@mhvk
Copy link
Contributor

mhvk commented Dec 19, 2022

OK, sorry about the linkcheck comment -- happy if that is unrelated!

Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Thank you!

@pllim pllim merged commit f879fa8 into astropy:main Dec 19, 2022
@lumberbot-app

This comment was marked as resolved.

meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request Dec 19, 2022
pllim added a commit to pllim/astropy that referenced this pull request Dec 19, 2022
Merge pull request astropy#14193 from saimn/fix-numpy124

Fix compat with Numpy 1.24

(cherry picked from commit f879fa8)
@saimn saimn deleted the fix-numpy124 branch December 19, 2022 20:49
@saimn
Copy link
Contributor Author

saimn commented Dec 19, 2022

There is indeed no v5.2.x on readthedocs; I think that should be latest or 5.2?

Should be 5.2, see #14196.

pllim added a commit that referenced this pull request Dec 19, 2022
Manual backport of #14193 to v5.0.x (Fix compat with Numpy 1.24)
pllim added a commit that referenced this pull request Dec 19, 2022
…193-on-v5.2.x

Backport PR #14193 on branch v5.2.x (Fix compat with Numpy 1.24)
@dhomeier dhomeier mentioned this pull request Dec 19, 2022
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Extra CI Run cron CI as part of PR no-changelog-entry-needed utils.masked 💤 backport-v5.0.x on-merge: backport to v5.0.x 💤 backport-v5.2.x on-merge: backport to v5.2.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test failures with Numpy 1.24
3 participants