Skip to content

BUG: Ensure gufuncs using structured dtypes can work with Masked - #16125

Merged
mhvk merged 2 commits into
astropy:mainfrom
mhvk:masked-fix-structured-dtype-ufunc
Feb 29, 2024
Merged

BUG: Ensure gufuncs using structured dtypes can work with Masked#16125
mhvk merged 2 commits into
astropy:mainfrom
mhvk:masked-fix-structured-dtype-ufunc

Conversation

@mhvk

@mhvk mhvk commented Feb 28, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Builds on #16120, since I think this became just a bit too involved for a backport. EDIT: now rebased, so just a single commit. Still a bit too much of a feature/api change for it to be backported, I think.

Fixes #16123

  • 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 this to the v6.1.0 milestone Feb 28, 2024
@github-actions

github-actions Bot commented Feb 28, 2024

Copy link
Copy Markdown
Contributor

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?
  • 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 mentioned this pull request Feb 28, 2024
Closed
@pllim

pllim commented Feb 28, 2024

Copy link
Copy Markdown
Member

#16120 is merged, please rebase, thanks!

@mhvk
mhvk force-pushed the masked-fix-structured-dtype-ufunc branch from 36e8c20 to eaf18bc Compare February 29, 2024 00:37
@mhvk

mhvk commented Feb 29, 2024

Copy link
Copy Markdown
Contributor Author

Thanks, rebased! @neutrinoceros - now this last one is ready for review. And hopefully it means your PR can be bug-free...

@neutrinoceros neutrinoceros mentioned this pull request Feb 29, 2024
Draft
1 task
@neutrinoceros

neutrinoceros commented Feb 29, 2024

Copy link
Copy Markdown
Contributor

First of all: I confirm that this is fixing the last non-local bug seen in #16070 !
Now let me review this thoroughly...

neutrinoceros
neutrinoceros reviewed Feb 29, 2024
View reviewed changes

@neutrinoceros neutrinoceros left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, only a couple questions before I'm ready to approve !

Comment thread astropy/utils/masked/core.py Resolved
Comment thread astropy/utils/masked/core.py Resolved
Comment thread astropy/utils/masked/core.py Resolved
Comment thread astropy/utils/masked/tests/test_functions.py Resolved
Comment thread astropy/utils/masked/tests/test_functions.py Resolved
Comment thread astropy/utils/masked/tests/test_functions.py Resolved
@mhvk
mhvk force-pushed the masked-fix-structured-dtype-ufunc branch from eaf18bc to c43bcc3 Compare February 29, 2024 15:37
mhvk
mhvk commented Feb 29, 2024
View reviewed changes
Comment thread astropy/utils/masked/tests/test_functions.py Resolved
mhvk
mhvk commented Feb 29, 2024
View reviewed changes
Comment thread astropy/utils/masked/tests/test_masked.py Resolved
@mhvk

mhvk commented Feb 29, 2024

Copy link
Copy Markdown
Contributor Author

OK, think this is done now! As you'll see inline, your review triggered further thoughts and additional tests!

neutrinoceros
neutrinoceros approved these changes Feb 29, 2024
View reviewed changes

@neutrinoceros neutrinoceros left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like how this patch has twice as much test code than user-facing code, and it's now good to go as far as I'm concerned. Thank you !

pllim
pllim approved these changes Feb 29, 2024
View reviewed changes

@pllim pllim left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think I am now your official gufuncs puppet approver. 😆

@mhvk feel free to merge. Thanks, all!

@mhvk

mhvk commented Feb 29, 2024

Copy link
Copy Markdown
Contributor Author

Yes, we should get @neutrinoceros merge privilege... Thanks, both!

@mhvk
mhvk merged commit 1fadf50 into astropy:main Feb 29, 2024
@mhvk
mhvk deleted the masked-fix-structured-dtype-ufunc branch February 29, 2024 17:05
@pllim

pllim commented Feb 29, 2024

Copy link
Copy Markdown
Member

Re: merge privilege -- We have a policy for that now in https://github.com/astropy/astropy-project/blob/main/policies/permissions.md . If @neutrinoceros stays on beyond the contract and becomes a core maintainer, he would automatically get that access. 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pllim pllim pllim approved these changes
@neutrinoceros neutrinoceros neutrinoceros approved these changes

Assignees

No one assigned

Labels

Bug utils.masked utils

Projects

None yet

Milestone

v6.1.0

Development

Successfully merging this pull request may close these issues.

BUG: MaskedNDArray.__array_ufunc__ breaks for erfa.atioq

3 participants

@mhvk @pllim @neutrinoceros