Skip to content

Only propagate info if the quantity stays effectively the same - #14253

Merged
mhvk merged 2 commits into
astropy:mainfrom
mhvk:quantity-do-not-copy-info-for-operations
Jan 5, 2023
Merged

Only propagate info if the quantity stays effectively the same#14253
mhvk merged 2 commits into
astropy:mainfrom
mhvk:quantity-do-not-copy-info-for-operations

Conversation

@mhvk

@mhvk mhvk commented Jan 4, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

This pull request is to address the fact that info gets copied from one quantity to another even when it makes little sense (such as in operations on quantities), slowing things done needlessly and possibly leading to confusion.

Fixes #5047, #14248

Also addresses #14244 (comment)

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.

@mhvk mhvk added units API change PRs and issues that change an existing API, possibly requiring a deprecation period labels Jan 4, 2023
@mhvk mhvk added this to the v5.3 milestone Jan 4, 2023
@mhvk
mhvk requested a review from taldcroft January 4, 2023 19:59
@mhvk mhvk mentioned this pull request Jan 4, 2023
Open
taldcroft
taldcroft reviewed Jan 5, 2023
View reviewed changes

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

This looks generally good with some small comments.

My only big-picture question is about performance. How much improvement does this bring for the covered cases? Conversely, are there any possible performance regressions in other areas?

Comment thread astropy/units/tests/test_quantity_info.py Resolved
Comment thread astropy/units/quantity.py Resolved
Comment thread astropy/units/tests/test_quantity_info.py Resolved
@mhvk

mhvk commented Jan 5, 2023

Copy link
Copy Markdown
Contributor Author

@taldcroft - on the performance, it is quite an improvement:

In [2]: q = [2,4]*u.percent

In [3]: q2 = 1*u.percent

In [4]: %timeit q + q2
8.27 µs ± 101 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)
--> reduced a little to 7.47 µs ± 71.8 ns
In [5]: q.info.name = 'q'

In [6]: %timeit q + q2
13 µs ± 153 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)
--> reduced quite a bit to 7.62 µs ± 103 ns

@mhvk
mhvk force-pushed the quantity-do-not-copy-info-for-operations branch from 79df221 to d95e68f Compare January 5, 2023 16:39
taldcroft
taldcroft approved these changes Jan 5, 2023
View reviewed changes

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

OK, looks good, pending CI passing. Once that happens feel free check the CI box in the description and merge.

@mhvk
mhvk merged commit f1620ba into astropy:main Jan 5, 2023
@mhvk
mhvk deleted the quantity-do-not-copy-info-for-operations branch January 5, 2023 17:06
@mhvk mhvk mentioned this pull request Jan 5, 2023
Closed
@pllim pllim mentioned this pull request Mar 10, 2023
Closed
@mhvk mhvk mentioned this pull request Mar 12, 2023
Merged
davidgegiya added a commit to davidgegiya/agent-orchestrator that referenced this pull request Nov 8, 2026
@davidgegiya
Add astropy #14253: When should info be linked to a new object?
2d3624b 
Pin dd2304672cdf4ea1b6f124f9f22ec5069a13c9f5 and its regression selectors for reproducible runs.

Source: astropy/astropy#14253
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@taldcroft taldcroft taldcroft approved these changes

Assignees

No one assigned

Labels

API change PRs and issues that change an existing API, possibly requiring a deprecation period Performance units

Projects

None yet

Milestone

v5.3

Development

Successfully merging this pull request may close these issues.

When should info be linked to a new object?

2 participants

@mhvk @taldcroft