Skip to content

Backport PR #19129 on branch v7.2.x (BUG: avoid mutation of shape attibutes (modeling)) - #19150

Merged
pllim merged 1 commit into
astropy:v7.2.xfrom
meeseeksmachine:auto-backport-of-pr-19129-on-v7.2.x
Jan 14, 2026
Merged

Backport PR #19129 on branch v7.2.x (BUG: avoid mutation of shape attibutes (modeling))#19150
pllim merged 1 commit into
astropy:v7.2.xfrom
meeseeksmachine:auto-backport-of-pr-19129-on-v7.2.x

Conversation

@meeseeksmachine

@meeseeksmachine meeseeksmachine commented Jan 13, 2026

Copy link
Copy Markdown

Backport PR #19129: BUG: avoid mutation of shape attibutes (modeling)

@meeseeksmachine
meeseeksmachine requested a review from a team as a code owner January 13, 2026 20:45
@lumberbot-app lumberbot-app Bot added this to the v7.2.1 milestone Jan 13, 2026
@github-actions

github-actions Bot commented Jan 13, 2026

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.

pllim
pllim reviewed Jan 13, 2026
View reviewed changes
Comment thread astropy/modeling/tests/test_parameters.py Outdated
param = np.reshape(param, (5,))
# Reshape success
param.shape = (2, 2)
param = np.reshape(param, (2, 2))

@pllim pllim Jan 13, 2026

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.

@mhvk what is the correct way to fix this? This is not correct because then param.value will error out. I think we missed this on main. I was hoping to fix in the backport and then forward port the backport fix...

@mhvk mhvk Jan 14, 2026

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.

Hmm, I think this change was a mistake. Param is not a subclass of ndarray so in principle it can have different semantics. The quick-fix solution would be to keep setting the shape here in the tests, and inside the setter for Parameter.shape use np.reshape -- this is at:

astropy/astropy/modeling/parameters.py

Lines 461 to 467 in dc7afdd

@shape.setter
def shape(self, value):
if isinstance(self.value, np.generic):
if value not in ((), (1,)):
raise ValueError("Cannot assign this shape to a scalar quantity")
else:
self.value.shape = value

cc @neutrinoceros

Longer term, I think we should implement __array_function__ on Parameter, or, maybe better, have it inherit from ShapedLikeNDArray and define an appropriate _apply. With that, functions like np.reshape, np.ravel, etc., will just work.

@pllim pllim Jan 14, 2026

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.

Thanks! I think that works great, at least when testing locally.

Can you please review the whole diff in this PR and see if they all still make sense altogether? 🙏

@neutrinoceros neutrinoceros Jan 14, 2026

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.

moved to #19152

@pllim
pllim force-pushed the auto-backport-of-pr-19129-on-v7.2.x branch from 03447d3 to 2f86fad Compare January 14, 2026 02:39
@pllim pllim added backport-main on-merge: backport to main and removed backport-v7.2.x on-merge: backport to v7.2.x labels Jan 14, 2026
@neutrinoceros

neutrinoceros commented Jan 14, 2026

Copy link
Copy Markdown
Contributor

Maybe there's been a misunderstanding. I never said #19129 would fix all errors in modeling, I just wanted to sort out the most trivial ones. While additional changes here make sense and are ultimately necessary too, I don't think we should add new content on a backport PR. Since it's still very early morning in the US now, I'll open a separate PR with your commits (retaining ownership) myself.

@neutrinoceros neutrinoceros mentioned this pull request Jan 14, 2026
Merged
1 task
@neutrinoceros

neutrinoceros commented Jan 14, 2026

Copy link
Copy Markdown
Contributor

done in #19152
I'm going to remove these commits from this PR. I hope that's ok.

@neutrinoceros
neutrinoceros force-pushed the auto-backport-of-pr-19129-on-v7.2.x branch from 2f86fad to 723548c Compare January 14, 2026 10:31
mhvk
mhvk approved these changes Jan 14, 2026
View reviewed changes

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

Yes, makes some sense to just backport what went in main. Approving but will let @pllim have the final say.

@pllim

pllim commented Jan 14, 2026

Copy link
Copy Markdown
Member

Yes, you caught me being lazy. Thanks for the proper follow-up!

@pllim
pllim merged commit 6c53e5d into astropy:v7.2.x Jan 14, 2026
69 of 72 checks passed
@pllim pllim removed the backport-main on-merge: backport to main label Jan 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pllim pllim pllim approved these changes
@neutrinoceros neutrinoceros neutrinoceros left review comments
@mhvk mhvk mhvk approved these changes

Assignees

No one assigned

Labels

Bug modeling no-changelog-entry-needed numpy-dev

Projects

None yet

Milestone

v7.2.1

Development

Successfully merging this pull request may close these issues.

4 participants

@meeseeksmachine @neutrinoceros @pllim @mhvk