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.

Sign up for GitHub

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

Jump to bottom

Add np.max and np.min to SUBCLASS_SAFE_FUNCTIONS #14484

Merged
merged 4 commits into from
Mar 2, 2023
Merged

Add np.max and np.min to SUBCLASS_SAFE_FUNCTIONS #14484

merged 4 commits into from
Mar 2, 2023

Conversation

larrybradley
Copy link
Member

In numpy/numpy#23302, numpy deprecated np.amax and np.amin in favor of np.max and np.min. This PR adds np.max and np.min to the Quantity SUBCLASS_SAFE_FUNCTIONS list.

Fixes #14483

CC: @mhvk, @pllim

@github-actions
Copy link

github-actions bot commented Mar 2, 2023

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see "When to rebase and squash commits".
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

@pllim pllim added this to the v5.0.6 milestone Mar 2, 2023
@pllim pllim 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 Mar 2, 2023
@larrybradley larrybradley requested a review from mhvk March 2, 2023 18:49
@pllim
Copy link
Member

pllim commented Mar 2, 2023

Thanks! Any chance you can add your minimal example as a test? 😸

@larrybradley
Copy link
Member Author

@pllim I did add some tests, but I think everything in the SUBCLASS_SAFE_FUNCTIONS list already gets tested.

@bmorris3 bmorris3 mentioned this pull request Mar 2, 2023
Merged
9 tasks
@mhvk mhvk mentioned this pull request Mar 2, 2023
Closed
mhvk
mhvk reviewed Mar 2, 2023
View reviewed changes
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.

Looks good, except we also need round_ and a similar update for Masked. Since I was rather confused about what is and what is not deprecated, it seemed easier to just do it, so I'll push 2 extra commits to your branch in a minute.

astropy/units/quantity_helper/function_helpers.py Show resolved Hide resolved
astropy/units/tests/test_quantity_non_ufuncs.py Show resolved Hide resolved
mhvk
mhvk reviewed Mar 2, 2023
View reviewed changes
astropy/utils/masked/tests/test_function_helpers.py
@@ -579,6 +579,12 @@ def check(self, function, *args, method=None, **kwargs):
x = getattr(self.ma, method)(*args, **kwargs)
assert_masked_equal(o, x)

def test_max(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Just in case you're curious, for Masked all functions in fromnumeric.py are included automatically as subclass safe functions, hence unlike for Quantity, there was no need to add them explicitly.

@mhvk
Copy link
Contributor

mhvk commented Mar 2, 2023

OK, pushed my additions. Hopefully the -dev test will now pass (and I didn't break anything else...)

@mhvk
Copy link
Contributor

mhvk commented Mar 2, 2023

I guess I shouldn't approve anymore since I contributed, though should all be OK. @pllim??

@larrybradley
Copy link
Member Author

Thanks, @mhvk!

pllim
pllim approved these changes Mar 2, 2023
View reviewed changes
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.

LGTM as long as CI passes. Thanks!

@pllim pllim merged commit aca313f into astropy:main Mar 2, 2023
@lumberbot-app

This comment was marked as resolved.

Sign in to view
meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request Mar 2, 2023
@pllim @meeseeksmachine
Backport PR astropy#14484: Add np.max and np.min to SUBCLASS_SAFE_FUN…
cfc4f81 
…CTIONS
@meeseeksmachine meeseeksmachine mentioned this pull request Mar 2, 2023
Merged
@pllim
Copy link
Member

pllim commented Mar 2, 2023

@larrybradley , do you want to do the manual backport?

@larrybradley larrybradley deleted the unit-safe-functions branch March 2, 2023 22:45
@mhvk
Copy link
Contributor

mhvk commented Mar 2, 2023

Yay for the tests passing, but sad that the backport is a problem...

We may have to do something similar soon -- numpy/numpy#23314 -- but thankfully it is not too hard (and I do agree with deprecatinging aliases).

larrybradley added a commit to larrybradley/astropy that referenced this pull request Mar 2, 2023
@larrybradley
Backport PR astropy#14484: Add np.max and np.min to SUBCLASS_SAFE_FUN…
d42cc9e 
…CTIONS
@larrybradley
Copy link
Member Author

I've manually backported: #14488

@pllim pllim mentioned this pull request Mar 2, 2023
Merged
pllim added a commit that referenced this pull request Mar 2, 2023
@pllim
Merge pull request #14487 from meeseeksmachine/auto-backport-of-pr-14…
ef8d988 
…484-on-v5.2.x

Backport PR #14484 on branch v5.2.x (Add np.max and np.min to SUBCLASS_SAFE_FUNCTIONS)
pllim added a commit that referenced this pull request Mar 2, 2023
@pllim
Merge pull request #14488 from larrybradley/auto-backport-of-pr-14484…
4bab3a8 
…-on-v5.0.x

Backport PR #14484 on branch v5.0.x (Add np.max and np.min to SUBCLASS_SAFE_FUNCTIONS)
@mhvk mhvk mentioned this pull request Mar 9, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhvk mhvk mhvk left review comments

@pllim pllim pllim approved these changes

Assignees
No one assigned
Labels
no-changelog-entry-needed units 💤 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
Milestone
v5.0.6
Development

Successfully merging this pull request may close these issues.

New Quantity warning starting with yesterday's numpy-dev
3 participants
@larrybradley @pllim @mhvk