Skip to content

Fix bug in fitting and accessing the parameter array when setting Parameter.value - #16710

Closed
astrofrog wants to merge 6 commits into
astropy:mainfrom
astrofrog:fix-parameters-to-array
Closed

Fix bug in fitting and accessing the parameter array when setting Parameter.value#16710
astrofrog wants to merge 6 commits into
astropy:mainfrom
astrofrog:fix-parameters-to-array

Conversation

@astrofrog

@astrofrog astrofrog commented Jul 12, 2024

Copy link
Copy Markdown
Member

It is currently possible to change the shape of parameters on a model by setting Parameter.value:

>>> from astropy.modeling.models import Polynomial1D
>>> poly = Polynomial1D(2, c0=[1, 2], c1=[2, 3], c2=[3, 4])
>>> poly.c0.value = 1
>>> poly.c1.value = 2
>>> poly.c2.value = 3

This is actually really helpful in order to solve #16593. However, one runs into a couple of bugs when doing this:

>>> poly.parameters
array([1., 1., 2., 2., 3., 3.])

this is incorrect, as it should now be [1, 2, 3]. And when fitting:

>>> from astropy.modeling.fitting import LevMarLSQFitter
>>> poly_result = LevMarLSQFitter()(poly, [1, 2, 3, 4, 5], [4, 12, 26, 46, 72])
...
ValueError: shape mismatch: objects cannot be broadcast to a single shape.  Mismatch is between arg 0 with shape (5,) and arg 1 with shape (2,). self input argument 'x' of shape (5,) cannot be broadcast with parameter 'c0' of shape (2,).

These two bugs have the same root cause - Model._parameters_to_array assumes that the parameter shape never changes. The fix is simple: when calling _parameters_to_array, we call _initialize_slices, but we can be smart and avoid re-allocating self._parameters if the total size hasn't changed for performance.

In the process I noticed some code duplication which I've removed in #16709

  • 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 added the Bug label Jul 12, 2024
@astrofrog
astrofrog requested a review from a team as a code owner July 12, 2024 10:30
@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 added this to the v6.1.2 milestone Jul 12, 2024
@astrofrog astrofrog mentioned this pull request Jul 12, 2024
Merged
1 task
@astrofrog

astrofrog commented Jul 12, 2024

Copy link
Copy Markdown
Member Author

With this fix, we can remove the terrible hacks from #16696 that were used to make models with different parameter shapes: ace17a3

This was referenced Jul 12, 2024
Merged
Closed
neutrinoceros
neutrinoceros reviewed Jul 15, 2024
View reviewed changes
Comment thread astropy/modeling/tests/test_core.py Outdated
poly.c1.value = 2
poly.c2.value = 3

assert_allclose(poly.parameters, [1, 2, 3])

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

assert_array_equal seems more appropriate to me since there no floating point arithmetic is involved (and 1, 2 and 3 have exact floating point representations)

@astrofrog astrofrog Jul 15, 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.

Done

neutrinoceros
neutrinoceros reviewed Jul 15, 2024
View reviewed changes
Comment thread astropy/modeling/tests/test_fitters.py
neutrinoceros
neutrinoceros reviewed Jul 15, 2024
View reviewed changes
Comment thread astropy/modeling/core.py
param_metrics[name]["size"] = param_size
total_size += param_size
self._parameters = np.empty(total_size, dtype=np.float64)
if self._parameters is None or self._parameters.size != total_size:

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

I would add a comment here to explain the reason this is conditional (perfs)

@astrofrog astrofrog Jul 15, 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.

Done

neutrinoceros
neutrinoceros reviewed Jul 15, 2024
View reviewed changes
Comment thread astropy/modeling/core.py Outdated
param_metrics[name]["size"] = param_size
total_size += param_size
self._parameters = np.empty(total_size, dtype=np.float64)
if self._parameters is None or self._parameters.size != total_size:

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

same here

@astrofrog astrofrog Jul 15, 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.

Done

@astrofrog
astrofrog force-pushed the fix-parameters-to-array branch from c99d6b2 to 4387cf0 Compare July 23, 2024 22:35
@astrofrog astrofrog modified the milestones: v6.1.2, v6.1.3 Jul 25, 2024
@astrofrog astrofrog mentioned this pull request Aug 9, 2024
Merged
1 task
@nden nden closed this in #16812 Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Assignees

No one assigned

Labels

Bug modeling

Projects

None yet

Milestone

v6.1.3

Development

Successfully merging this pull request may close these issues.

2 participants

@astrofrog @neutrinoceros