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.

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

Add speed-up values to 5.2 whats new #14094

Merged
merged 1 commit into from Dec 8, 2022

Conversation

maxnoe
Copy link
Member

@maxnoe maxnoe commented Dec 2, 2022

Description

This pull request is to address ...

Fixes #

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

  • 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 astropy-bot check might be missing; 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.

@github-actions github-actions bot added the Docs label Dec 2, 2022
@eerovaher eerovaher added no-changelog-entry-needed skip-basebranch-check Skip base branch check for direct backports labels Dec 2, 2022
@eerovaher eerovaher added this to the v5.2 milestone Dec 2, 2022
docs/whatsnew/5.2.rst Outdated Show resolved Hide resolved
@eerovaher
Copy link
Member

There's a line that is a bit too long and the commits should be squashed, but the phrasing looks good enough to merge.

@@ -116,6 +116,9 @@ Performance Improvements
To help speed up coordinate transformations, several performance improvements
were implemented, mainly concerning validity checks in ``Angle`` and
how ``FrameAttributes`` are accessed.
Performance improvements will vary between different tasks, e.g. the speedup of
Copy link
Member

Choose a reason for hiding this comment

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

Before you squash... 😸

Suggested change
Performance improvements will vary between different tasks, e.g. the speedup of
Performance improvements will vary between different tasks, e.g., the speedup of

Copy link
Contributor

Choose a reason for hiding this comment

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

🤔

@pllim pllim requested a review from astrofrog December 2, 2022 22:20
@pllim
Copy link
Member

pllim commented Dec 6, 2022

@astrofrog , is it too late for v5.2 ?

@astrofrog
Copy link
Member

No, changes/improvements to docs are fine during the RC stage.

@pllim
Copy link
Member

pllim commented Dec 7, 2022

@maxnoe , are you interested to wrap this up? If not I can take over, pls lemme know. Thanks!

@maxnoe
Copy link
Member Author

maxnoe commented Dec 7, 2022

Sorry, yes, will squash

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.

Thanks, all!

@pllim
Copy link
Member

pllim commented Dec 7, 2022

@eerovaher and @dhomeier , do you want one last look or can I merge as-is? Thanks!

@dhomeier
Copy link
Contributor

dhomeier commented Dec 7, 2022

I did not understand the grammar of a trailing comma after for example, but maybe that's AE vs. BE idiom.

@pllim
Copy link
Member

pllim commented Dec 7, 2022

ApJ used to enforce trailing comma for i.e. and e.g.. Lemme double check.

@pllim
Copy link
Member

pllim commented Dec 7, 2022

Well, I cannot search effectively with their new UI but they do use trailing comma after e.g. in their own style guide narrative, so I guess it still stands: https://journals.aas.org/aas-style-guide/

docs/whatsnew/5.2.rst Outdated Show resolved Hide resolved
@dhomeier
Copy link
Contributor

dhomeier commented Dec 7, 2022

Well, looks like they are sold on it. My OALDC OTOH is a 1989 edition and does not even generally endorse the Oxford(!) comma. 😂

@dhomeier
Copy link
Contributor

dhomeier commented Dec 7, 2022

Yea, it's an Americanism https://en.wiktionary.org/wiki/e.g.#Usage_notes

@dhomeier
Copy link
Contributor

dhomeier commented Dec 7, 2022

Humm,

/Users/runner/work/astropy/astropy/.tox/.tox/bin/python: No module named tox

both on Actions and CircleCI??

@dhomeier
Copy link
Contributor

dhomeier commented Dec 7, 2022

pre-commit.ci autofix

@maxnoe
Copy link
Member Author

maxnoe commented Dec 7, 2022

And both here and on the other PR... seems to be some general issue.

Copy link
Member

@eerovaher eerovaher left a comment

Choose a reason for hiding this comment

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

Now it needs to be squashed.

@pllim pllim merged commit 349499e into astropy:v5.2.x Dec 8, 2022
@pllim
Copy link
Member

pllim commented Dec 8, 2022

Thanks, all!

@maxnoe maxnoe deleted the coord_perf_whatsnew branch December 8, 2022 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docs no-changelog-entry-needed skip-basebranch-check Skip base branch check for direct backports
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants