Skip to content

Broadcast frames over both data and attributes - #15121

Merged
mhvk merged 19 commits into
astropy:mainfrom
lpsinger:broadcast-frames
Oct 3, 2023
Merged

mhvk merged 19 commits into
astropy:mainfrom
lpsinger:broadcast-frames

Conversation

@lpsinger

@lpsinger lpsinger commented Aug 3, 2023

Copy link
Copy Markdown
Contributor

Support Numpy broadcasting rules for frame attributes.

Fixes #5982, fixes #8812, fixes #11184, fixes #12965, fixes #14383.

@github-actions

github-actions Bot commented Aug 3, 2023 •

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 "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 v6.0 milestone Aug 3, 2023
@lpsinger

lpsinger commented Aug 3, 2023

Copy link
Copy Markdown
Contributor Author

@pllim, is this truly a breaking change, or could this be regarded as a bug fix, to go in earlier than v6?

@pllim

pllim commented Aug 3, 2023

Copy link
Copy Markdown
Member

@lpsinger , your change log fragment says "feature" and we do not backport features, just bug fixes. If you want to rename the fragment, please squash your commits. I'll let coordinates maintainers decide though whether this is bug fix or new feature: @StuartLittlefair , @adrn , @eteq , @eerovaher , @mhvk

@lpsinger

lpsinger commented Aug 3, 2023

Copy link
Copy Markdown
Contributor Author

Done.

@pllim pllim added the Bug label Aug 3, 2023
@mhvk

mhvk commented Aug 3, 2023

Copy link
Copy Markdown
Contributor

I think "feature" is appropriate here! I won't really have time to look in any detail until next week, but this is a very long-standing wish-list item! Still, I think it is involved enough -- and an API change, really (even if one we have wanted for a long time) that it is better not to backport.

@mhvk mhvk added Bug API change PRs and issues that change an existing API, possibly requiring a deprecation period and removed Bug Enhancement labels Aug 3, 2023
@pllim

pllim commented Aug 4, 2023

Copy link
Copy Markdown
Member

Would you like me to switch the change log entry back to "feature"?

I'll leave that you y'all because bug fixes are allowed in feature release, so as far as I am concerned, it does not matter. But if you do end up renaming, please squash again. Sorry for the inconvenience!

Side question, do you have a sense of when astropy 6 will be released?

By the end of 2023, see https://github.com/astropy/astropy/wiki/Release-Calendar

@pllim

pllim commented Aug 4, 2023

Copy link
Copy Markdown
Member

p.s. Kudos to fixing not one, but five issues in one go. Thanks! 👏

@lpsinger
lpsinger force-pushed the broadcast-frames branch 2 times, most recently from aa1989c to 1d45f88 Compare August 4, 2023 14:20
@lpsinger

lpsinger commented Aug 4, 2023

Copy link
Copy Markdown
Contributor Author

I made this a "feature" again and I added a bit more motivation to the changelog entry.

I could see this as meriting a writeup in "What's New." What do you think?

@pllim

pllim commented Aug 4, 2023

Copy link
Copy Markdown
Member

Sure! I think broadcasting is very useful and should be in what's new. Thanks!

@lpsinger

lpsinger commented Aug 4, 2023

Copy link
Copy Markdown
Contributor Author

Side question, do you have a sense of when astropy 6 will be released?

By the end of 2023, see https://github.com/astropy/astropy/wiki/Release-Calendar

I see. I plan on using this capability (whether "feature" or "bug fix") in my multi-observatory observation planning project. The time horizon of that project means that I have a vested interest in treating this as a "bug" rather than a "feature" 😃

@pllim

pllim commented Aug 4, 2023

Copy link
Copy Markdown
Member

@lpsinger , what is your deadline?

@lpsinger

lpsinger commented Aug 4, 2023

Copy link
Copy Markdown
Contributor Author

@lpsinger , what is your deadline?

I'd like to submit a paper with working code using a stable Astropy release by about October, which is when the grant that is funding the work ends.

@pllim

pllim commented Aug 4, 2023

Copy link
Copy Markdown
Member

If you really need this in a bug fix release... is there a way to separate out the backward compatible stuff into a separate PR that can be backported?

@lpsinger

lpsinger commented Aug 4, 2023

Copy link
Copy Markdown
Contributor Author

If you really need this in a bug fix release... is there a way to separate out the backward compatible stuff into a separate PR that can be backported?

Where would you divide the line? It seems pretty atomic to me.

What about something like a feature flag (perhaps implemented as science state) to toggle the broadcasting on or off? Or a warning.

@pllim

pllim commented Aug 4, 2023

Copy link
Copy Markdown
Member

@mhvk et al., what do you think?

@lpsinger

lpsinger commented Aug 4, 2023

Copy link
Copy Markdown
Contributor Author

I wrote an extended example for "What's New".

@lpsinger

lpsinger commented Oct 3, 2023

Copy link
Copy Markdown
Contributor Author

Doctest failure sadly looks real:

Fixed now. There is a remaining test failure, but it looks like it is not related to this PR.

@mhvk mhvk 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.

Super, looks all great now!

@mhvk
mhvk dismissed StuartLittlefair’s stale review October 3, 2023 12:24

Requested changes have been implemented

@mhvk
mhvk enabled auto-merge (squash) October 3, 2023 12:26
@mhvk
mhvk merged commit 00e64a9 into astropy:main Oct 3, 2023
@lpsinger
lpsinger deleted the broadcast-frames branch October 3, 2023 12:53
@pllim

pllim commented Oct 3, 2023 •

Copy link
Copy Markdown
Member

Thank you, all!

Re: #15121 (comment)

@lpsinger , how do you like to proceed here. Would an alpha be needed before we push out a regular RC as per https://github.com/astropy/astropy/wiki/Release-Calendar ?

Regardless, your changes would be released as part of astropy v6.0 and they would be available in the dev nightly wheel probably by tomorrow.

@lpsinger

lpsinger commented Oct 3, 2023

Copy link
Copy Markdown
Contributor Author

@lpsinger , how do you like to proceed here. Would an alpha be needed before we push out a regular RC as per https://github.com/astropy/astropy/wiki/Release-Calendar ?

The grant funding is over, and it was my fault due to getting stuck on the iterations for this PR. I don't think it matters. I can definitely work with the nightly builds in the mean time. But thanks for entertaining the possibility of an alpha release.

@pllim

pllim commented Oct 3, 2023

Copy link
Copy Markdown
Member

Okay. We will proceed normally then. Thank you!

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

Labels

API change PRs and issues that change an existing API, possibly requiring a deprecation period Bug coordinates whatsnew-needed

Projects

None yet

6 participants