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

Ensure mask calculation is correct also for multi-op ufunc with scalars #15450

Merged
merged 1 commit into from Oct 10, 2023
Merged

Ensure mask calculation is correct also for multi-op ufunc with scalars #15450

merged 1 commit into from Oct 10, 2023

Conversation

mhvk
Copy link
Contributor

@mhvk mhvk commented Oct 7, 2023 •
edited by pllim

Description

This pull request is to address a small bug where if a ufunc with more than 2 scalar Masked arguments was used, the resulting mask could not be calculated.

Found in #15231

  • By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

@mhvk mhvk added Bug utils utils.masked 💤 backport-v5.0.x on-merge: backport to v5.0.x 💤 backport-v5.3.x on-merge: backport to v5.3.x labels Oct 7, 2023
@mhvk mhvk added this to the v5.0.9 milestone Oct 7, 2023
@github-actions
Copy link

github-actions bot commented Oct 7, 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 instructions for rebase and squash.
  • 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.

@mhvk mhvk added Bug and removed Bug labels Oct 7, 2023
@mhvk mhvk mentioned this pull request Oct 9, 2023
Merged
1 task
@nstarman nstarman self-requested a review October 10, 2023 13:08
@mhvk
Copy link
Contributor Author

mhvk commented Oct 10, 2023

@nstarman - would you be able to have a look at this one as well? It is a really simple change, so should be easy. Thanks!

nstarman
nstarman reviewed Oct 10, 2023
View reviewed changes
docs/changes/utils/15450.bugfix.rst
Comment on lines +1 to +2
Ufuncs with more than 2 operands (such as ``erfa.dtf2d``) now work
also if all inputs are scalars and more than two inputs have masks.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Ufuncs with more than 2 operands (such as ``erfa.dtf2d``) now work
also if all inputs are scalars and more than two inputs have masks.
Ufuncs with more than 2 operands (such as ``erfa.dtf2d``) now also
work if all inputs are scalars and more than two inputs have masks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to change this, but perhaps only if there is also a code change? I think there is no worry about zip.

astropy/utils/masked/tests/test_functions.py
"utc", iy.unmasked, im.unmasked, idy.unmasked, ihr.unmasked, imn, isc
)
expected_mask = np.array([True, False, True])
for res, res0, exp in zip(result, result0, expected):
Copy link
Member

Choose a reason for hiding this comment

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

Aren't these different lengths? So it's not iterating over all of result? As in won't this fail?

Suggested change
for res, res0, exp in zip(result, result0, expected):
for res, res0, exp in zip(result, result0, expected, strict=True):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is OK: all of result, result0, and expected are output from the ufunc, which returns a tuple of 3 elements, jd1, jd2, and status, each of which can be an array or scalar.

Copy link
Member

Choose a reason for hiding this comment

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

But then zipping them together means the zip only goes to end of the shortest iterator, which is result0, right? Equivalent to result[0]? So the for loop is only 1 element?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, result0 is a tuple with 3 entries, it is the same as (jd1[0], jd2[0], stat[0]) where (jd1, jd2, stat) would be in result. The ufunc always gives back a tuple of three (I think my nomenclature may have not been very helpful here!).

Copy link
Member

Choose a reason for hiding this comment

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

Ah! Good. So then each element of result0 is broadcast against each element of result and everything works. Gotcha.

astropy/utils/masked/tests/test_functions.py
ihr = Masked([11, 12, 13], mask=[False, False, True])
imn = np.array([50, 51, 52])
isc = np.array([12.5, 13.6, 14.7])
result = erfa_ufunc.dtf2d("utc", iy, im, idy, ihr, imn, isc)
Copy link
Member

Choose a reason for hiding this comment

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

Are the later elements ever tested? See comments on the zip unpacking below.

nstarman
nstarman approved these changes Oct 10, 2023
View reviewed changes
Copy link
Member

@nstarman nstarman left a comment

Choose a reason for hiding this comment

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

LGTM! Might want to clean up variable names / add comments in the tests, but it's a minor point.

@mhvk
Copy link
Contributor Author

mhvk commented Oct 10, 2023

I think I'll just merge given other time constraints... Thanks for the quick review!!!

@mhvk mhvk merged commit f641d16 into astropy:main Oct 10, 2023
31 checks passed
@mhvk mhvk deleted the masked-multi-op-ufunc-with-scalars branch October 10, 2023 18:00
@lumberbot-app

This comment was marked as resolved.

Sign in to view
meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request Oct 10, 2023
@mhvk @meeseeksmachine
Backport PR astropy#15450: Ensure mask calculation is correct also fo…
20e5bea 
…r multi-op ufunc with scalars
@meeseeksmachine meeseeksmachine mentioned this pull request Oct 10, 2023
Merged
pllim added a commit that referenced this pull request Oct 10, 2023
@pllim
Merge pull request #15458 from meeseeksmachine/auto-backport-of-pr-15…
df2390f 
…450-on-v5.3.x

Backport PR #15450 on branch v5.3.x (Ensure mask calculation is correct also for multi-op ufunc with scalars)
@pllim
Copy link
Member

pllim commented Oct 10, 2023

Need manual backport to v5.0.x. If you do not want to do that, then please change the milestone. Thanks!

mhvk added a commit to mhvk/astropy that referenced this pull request Oct 10, 2023
@mhvk
Backport PR astropy#15450: Ensure mask calculation is correct also fo…
dbd388a 
…r multi-op ufunc with scalars
@mhvk
Copy link
Contributor Author

mhvk commented Oct 10, 2023

Thanks, manual backport in #15460; it was not very tricky, thankfully.

@mhvk mhvk mentioned this pull request Oct 10, 2023
Merged
mhvk added a commit that referenced this pull request Oct 10, 2023
@mhvk
Merge pull request #15460 from mhvk/auto-backport-of-pr-15450-on-v5.0.x
ad940fd 
Backport PR #15450 on branch v5.0.x (Ensure mask calculation is correct also for multi-op ufunc with scalars)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nstarman nstarman nstarman approved these changes

Assignees
No one assigned
Labels
Bug utils.masked utils 💤 backport-v5.0.x on-merge: backport to v5.0.x 💤 backport-v5.3.x on-merge: backport to v5.3.x
Projects
None yet
Milestone
v5.0.9
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mhvk @pllim @nstarman