Skip to content

Ensure broadcast_arrays returns sequence also for single Masked instance - #16842

Merged
mhvk merged 1 commit into
astropy:mainfrom
mhvk:masked-broadcast-bug
Aug 19, 2024
Merged

Ensure broadcast_arrays returns sequence also for single Masked instance#16842
mhvk merged 1 commit into
astropy:mainfrom
mhvk:masked-broadcast-bug

Conversation

@mhvk

@mhvk mhvk commented Aug 17, 2024

Copy link
Copy Markdown
Contributor

This pull request is to address a small bug in the implementation of np.broadcast_arrays for a single Masked element, ensuring a sequence is also returned in that case.

@mhvk mhvk added this to the v6.1.3 milestone Aug 17, 2024
@github-actions

github-actions Bot commented Aug 17, 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 requested a review from neutrinoceros August 17, 2024 17:25
@mhvk mhvk mentioned this pull request Aug 18, 2024
Closed
1 task done
neutrinoceros
neutrinoceros reviewed Aug 18, 2024
View reviewed changes
Comment thread astropy/utils/masked/tests/test_function_helpers.py
self.check2(np.broadcast_arrays, subok=False)
# Regression test for bug for single array
ba = np.broadcast_arrays(self.ma, subok=True)
assert len(ba) == 1

@neutrinoceros neutrinoceros Aug 18, 2024

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 was initially confused by this first new assertion. I think it'd help readability to also assert the return value is indeed a tuple.

Suggested change
assert len(ba) == 1
assert isinstance(ba, tuple)
assert len(ba) == 1

neutrinoceros
neutrinoceros approved these changes Aug 18, 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.

nice to get the code both simpler and more correct. Just one simple suggestion to improve the test but otherwise LGTM. Thanks Marten !

@mhvk
mhvk force-pushed the masked-broadcast-bug branch from 38aeeed to 07bed38 Compare August 18, 2024 13:47
@mhvk

mhvk commented Aug 18, 2024

Copy link
Copy Markdown
Contributor Author

Thanks, I made the change and also rebased, so that #16843 is in and the full masked tests run for all cases. @pllim - could you do the honours given @neutrinoceros's approval?

@mhvk
mhvk force-pushed the masked-broadcast-bug branch from 07bed38 to 8ce1a20 Compare August 18, 2024 14:15
@mhvk
mhvk merged commit 11ca9f8 into astropy:main Aug 19, 2024
@mhvk
mhvk deleted the masked-broadcast-bug branch August 19, 2024 12:58
meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request Aug 19, 2024
@mhvk @meeseeksmachine
Backport PR astropy#16842: Ensure broadcast_arrays returns sequence a…
d929017 
…lso for single Masked instance
@meeseeksmachine meeseeksmachine mentioned this pull request Aug 19, 2024
Merged
@pllim

pllim commented Aug 19, 2024

Copy link
Copy Markdown
Member

Sorry about the delay. I wasn't working over the weekend. Glad Nathaniel able to help. Thanks, all!

pllim added a commit that referenced this pull request Aug 19, 2024
@pllim
Merge pull request #16849 from meeseeksmachine/auto-backport-of-pr-16…
41b65b9 
…842-on-v6.1.x

Backport PR #16842 on branch v6.1.x (Ensure broadcast_arrays returns sequence also for single Masked instance)
@mhvk

mhvk commented Aug 19, 2024

Copy link
Copy Markdown
Contributor Author

Sorry about the delay. I wasn't working over the weekend. Glad Nathaniel able to help. Thanks, all!

And you definitely shouldn't feel you have to, obviously!! I just ping'd because then I knew I wouldn't forget...

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
@neutrinoceros neutrinoceros neutrinoceros approved these changes

Assignees

No one assigned

Labels

Bug utils.masked utils

Projects

None yet

Milestone

v6.1.3

Development

Successfully merging this pull request may close these issues.

4 participants

@mhvk @pllim @nstarman @neutrinoceros