Skip to content

Reduce duplication of CompoundModel._initialize_slices - #16709

Merged
astrofrog merged 2 commits into
astropy:mainfrom
astrofrog:reduce-initialize-slices-duplication
Jul 23, 2024
Merged

Reduce duplication of CompoundModel._initialize_slices#16709
astrofrog merged 2 commits into
astropy:mainfrom
astrofrog:reduce-initialize-slices-duplication

Conversation

@astrofrog

@astrofrog astrofrog commented Jul 12, 2024

Copy link
Copy Markdown
Member

Noticed that CompoundModel._initialize_slices was duplicating most of the cost of Model._initialize_slices, so I've combined the two and all the tests seem to pass!

  • 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.

@astrofrog
astrofrog requested a review from a team as a code owner July 12, 2024 10:22
@github-actions

github-actions Bot commented Jul 12, 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.

@astrofrog astrofrog mentioned this pull request Jul 12, 2024
Closed
1 task
@astrofrog
astrofrog force-pushed the reduce-initialize-slices-duplication branch from 8a947fa to 38de69d Compare July 12, 2024 10:32
@astrofrog astrofrog added this to the v6.1.2 milestone Jul 12, 2024
@astrofrog

astrofrog commented Jul 12, 2024

Copy link
Copy Markdown
Member Author

Marking this as 6.1.2 because it would simplify a bug fix in #16710, but we can change to 7.0.0 if anyone has any strong feelings.

neutrinoceros
neutrinoceros reviewed Jul 12, 2024
View reviewed changes
Comment thread astropy/modeling/core.py Outdated
Comment on lines +2743 to +2744
if name not in param_metrics:
param_metrics[name] = {}

@neutrinoceros neutrinoceros Jul 12, 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.

This looks equivalent to

Suggested change
if name not in param_metrics:
param_metrics[name] = {}
param_metrics.setdefault(name, {})

but I also see that param_metrics is defined as a defaultdict(dict), so I don't understand why setting a default value is even needed.

https://github.com/astropy/astropy/blob/main/astropy/modeling/core.py#L2584

@astrofrog astrofrog Jul 12, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I dug into this further and found a place where param_metrics was set (incorrectly) to a regular dictionary, so I've fixed this.

neutrinoceros
neutrinoceros approved these changes Jul 14, 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.

LGTM now, thanks!

nden
nden approved these changes Jul 23, 2024
View reviewed changes

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

LGTM

@nden

nden commented Jul 23, 2024

Copy link
Copy Markdown
Contributor

I'm happy to merge this but am not sure what towncrier is complaining about.

@astrofrog

astrofrog commented Jul 23, 2024

Copy link
Copy Markdown
Member Author

I restarted and it passes

@nden

nden commented Jul 23, 2024

Copy link
Copy Markdown
Contributor

v6.1.2 is out, which is probably why it won't let me merge. Is it OK to create a v6.1.3 milestone or should I assign 7.0.0?

@astrofrog
astrofrog merged commit aee87f5 into astropy:main Jul 23, 2024
meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request Jul 23, 2024
@astrofrog @meeseeksmachine
Backport PR astropy#16709: Reduce duplication of CompoundModel._initi…
3d930dd 
…alize_slices
@meeseeksmachine meeseeksmachine mentioned this pull request Jul 23, 2024
Merged
@astrofrog astrofrog modified the milestones: v6.1.2, v7.0.0 Jul 23, 2024
@astrofrog

astrofrog commented Jul 23, 2024

Copy link
Copy Markdown
Member Author

I assigned 7.0.0 for now but will create 6.1.3 later. Strangely I didn't have issues merging even before changing the milestone.

@astrofrog astrofrog modified the milestones: v7.0.0, v6.1.2 Jul 23, 2024
@astrofrog

astrofrog commented Jul 23, 2024

Copy link
Copy Markdown
Member Author

I've changed it back to 6.1.2 to avoid confusion, will move it over to 6.1.3 when I do the post release steps.

pllim added a commit that referenced this pull request Jul 25, 2024
@pllim
Merge pull request #16762 from meeseeksmachine/auto-backport-of-pr-16…
bbb0c7f 
…709-on-v6.1.x

Backport PR #16709 on branch v6.1.x (Reduce duplication of CompoundModel._initialize_slices)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nden nden nden approved these changes
@neutrinoceros neutrinoceros neutrinoceros approved these changes
@perrygreenfield perrygreenfield Awaiting requested review from perrygreenfield
@WilliamJamieson WilliamJamieson Awaiting requested review from WilliamJamieson

Assignees

No one assigned

Labels

modeling no-changelog-entry-needed Ready-for-final-review Refactoring

Projects

None yet

Milestone

v6.1.2

Development

Successfully merging this pull request may close these issues.

4 participants

@astrofrog @nden @neutrinoceros @pllim