Skip to content

Fix removal of units from CompoundModel with * or / and fix fitting of these models#16678

Merged
pllim merged 6 commits intoastropy:mainfrom
astrofrog:fix-compound-without-units
Jul 10, 2024
Merged

Fix removal of units from CompoundModel with * or / and fix fitting of these models#16678
pllim merged 6 commits intoastropy:mainfrom
astrofrog:fix-compound-without-units

Conversation

@astrofrog
Copy link
Member

@astrofrog astrofrog commented Jul 4, 2024 •
edited
Loading

This fixes a bug that caused compound models created via CompoundModel.without_units_for_data to be incorrect if the compound expression included * or /, because the top-level parameters were decoupled from the parameters of the constituent models (see examples in #16675). The safer route is to instantiate a new CompoundModel rather than copy and modify. Example of incorrect behavior:

In [1]: from astropy.modeling.models import Gaussian1D

In [2]: from astropy import units as u

In [3]: g1 = Gaussian1D(amplitude=2 * u.Jy, stddev=4 * u.nm, mean=1000 * u.nm)

In [4]: g2 = Gaussian1D(amplitude=1 * u.Jy, stddev=2 * u.nm, mean=500 * u.nm)

In [5]: gg = g1 * g2

In [6]: gg
Out[6]: <CompoundModel(amplitude_0=2. Jy, mean_0=1000. nm, stddev_0=4. nm, amplitude_1=1. Jy, mean_1=500. nm, stddev_1=2. nm)>

In [11]: gg_nounit = gg.without_units_for_data(x=1 * u.nm, y=2 * u.Jy ** 2)[0]

In [13]: print(gg_nounit)
Model: CompoundModel
Inputs: ('x',)
Outputs: ('y',)
Model set size: 1
Expression: [0] * [1]
Components: 
    [0]: <Gaussian1D(amplitude=2., mean=1000., stddev=4.)>

    [1]: <Gaussian1D(amplitude=1., mean=500., stddev=2.)>
Parameters:
    amplitude_0 mean_0 stddev_0 amplitude_1 mean_1 stddev_1
    ----------- ------ -------- ----------- ------ --------
            2.0 1000.0      4.0         1.0  500.0      2.0

In [14]: gg_nounit.amplitude_0=5

In [15]: print(gg_nounit)
Model: CompoundModel
Inputs: ('x',)
Outputs: ('y',)
Model set size: 1
Expression: [0] * [1]
Components: 
    [0]: <Gaussian1D(amplitude=5., mean=1000., stddev=4.)>

    [1]: <Gaussian1D(amplitude=1., mean=500., stddev=2.)>
Parameters:
    amplitude_0 mean_0 stddev_0 amplitude_1 mean_1 stddev_1
    ----------- ------ -------- ----------- ------ --------
            5.0 1000.0      4.0         1.0  500.0      2.0

In [16]: gg_nounit.left.amplitude
Out[16]: Parameter('amplitude', value=2.0)

In [17]: gg_nounit.right.amplitude
Out[17]: Parameter('amplitude', value=1.0)

As you can see, gg_nounit.left.amplitude was actually incorrect, as it should be 5 now. This is now fixed with this PR.

A more serious issue that this caused was that fitting was in fact completely broken for compound models with units and with a * or / - the fitting simply returned the same model as the original unfit one. There were several tests of the fitting which were incorrectly written and assumed the fit and unfit output should be the same, which it should not be, so I have now adjusted those tests so that they pass again.

Fixes #16675

  • 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 4, 2024
@github-actions
Copy link
Contributor

github-actions bot commented Jul 4, 2024

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.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 4, 2024

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

@astrofrog astrofrog mentioned this pull request Jul 5, 2024
Closed
@astrofrog astrofrog changed the title Fix removal of units from CompoundModel Fix removal of units from CompoundModel with * or / and fix fitting of these models Jul 5, 2024
@astrofrog astrofrog added this to the v6.1.2 milestone Jul 5, 2024
@astrofrog astrofrog marked this pull request as ready for review July 5, 2024 09:37
@astrofrog astrofrog requested a review from a team as a code owner July 5, 2024 09:37
astrofrog added 5 commits July 5, 2024 22:27
@astrofrog
Fixed a bug in CompoundModel.without_units_for_data - before the top-…
608d798 
…level parameters on the CompoundModel were out of sync with the constituent models. It is cleaner to instantiate a new CompoundModel rather than try and modify a copy.
@astrofrog
Added regression test
63b6cd3
@astrofrog
Fix tests of fitting compound models with * or /
5a21651
@astrofrog
Added changelog entry
b05a9c1
@astrofrog
Increase number of iterations
2bd3dc3
@astrofrog astrofrog force-pushed the fix-compound-without-units branch from 97e4101 to 2bd3dc3 Compare July 5, 2024 21:27
perrygreenfield
perrygreenfield reviewed Jul 9, 2024
View reviewed changes
Copy link
Member

@perrygreenfield perrygreenfield left a comment

Choose a reason for hiding this comment

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

Does it make sense to have tests that employ fitting on degenerate models?

@astrofrog
Copy link
Member Author

astrofrog commented Jul 9, 2024

@perrygreenfield probably not, but I wanted to avoid changing the tests too much since they do work as long as we don't look at the degenerate parameters values?

@perrygreenfield
Copy link
Member

perrygreenfield commented Jul 9, 2024

I'd be happier with replacing these but someone else thinks they are OK, I can go with that. Otherwise I didn't see any other problems.

@astrofrog
Copy link
Member Author

astrofrog commented Jul 9, 2024

I'll have a think whether there is a way to rewrite the tests that would preserve their essence, and fail on main to make sure they act as a regression test.

@astrofrog
Copy link
Member Author

astrofrog commented Jul 9, 2024

What if we checked the values of combination of parameters? For example if and and b are degenerate, I might still be able to check the value of a*b?

@perrygreenfield
Copy link
Member

perrygreenfield commented Jul 9, 2024

Can we just fix one of the intercept values? That should remove the degeneracy shouldn't it?

@astrofrog
Copy link
Member Author

astrofrog commented Jul 9, 2024

Good idea about fixing one or more of the parameters, will investigate! (Might need to fix more than one for the compound models formed of four sub-models)

@astrofrog astrofrog force-pushed the fix-compound-without-units branch from daa4b1d to 197b51a Compare July 10, 2024 08:50
@astrofrog
Copy link
Member Author

astrofrog commented Jul 10, 2024

@perrygreenfield - ok I've now gone and fixed parameters - due to the nature of the model we need to fix all but two of the parameters, but I think this is fine and still tests what we need to test. There are no longer any degeneracies so we can check the parameter values.

perrygreenfield
perrygreenfield approved these changes Jul 10, 2024
View reviewed changes
Copy link
Member

@perrygreenfield perrygreenfield left a comment

Choose a reason for hiding this comment

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

LGTM. Good fix!

@pllim
Copy link
Member

pllim commented Jul 10, 2024

devdeps failure unrelated, so merging. Thanks, all!

@pllim pllim merged commit cc6294c into astropy:main Jul 10, 2024
meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request Jul 10, 2024
@astrofrog @meeseeksmachine
Backport PR astropy#16678: Fix removal of units from CompoundModel wi…
f0d4545 
…th * or / and fix fitting of these models
@meeseeksmachine meeseeksmachine mentioned this pull request Jul 10, 2024
Merged
pllim added a commit that referenced this pull request Jul 10, 2024
@pllim
Merge pull request #16699 from meeseeksmachine/auto-backport-of-pr-16…
74d3a34 
…678-on-v6.1.x

Backport PR #16678 on branch v6.1.x (Fix removal of units from CompoundModel with * or / and fix fitting of these models)
@astrofrog astrofrog mentioned this pull request Jul 12, 2024
Merged
1 task
d-giles referenced this pull request in d-giles/astropy-abdufork Jul 26, 2024
@astrofrog @d-giles
Fix removal of units from CompoundModel with * or / and fix fitting o…
25ffdea 
…f these models (#16678)

* Fixed a bug in CompoundModel.without_units_for_data - before the top-level parameters on the CompoundModel were out of sync with the constituent models. It is cleaner to instantiate a new CompoundModel rather than try and modify a copy.

* Added regression test

* Fix tests of fitting compound models with * or /

* Added changelog entry

* Increase number of iterations

* Fix parameters to avoid degeneracies and compare parameter values to those expected.
@dhomeier dhomeier mentioned this pull request Sep 19, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@perrygreenfield perrygreenfield perrygreenfield approved these changes

@nden nden Awaiting requested review from nden

@WilliamJamieson WilliamJamieson Awaiting requested review from WilliamJamieson

Assignees

No one assigned

Labels

Bug modeling Ready-for-final-review

Projects

None yet

Milestone

v6.1.2

Development

Successfully merging this pull request may close these issues.

The behaviour of CompoundModel.evaluate and CompoundModel.__call__ are different

4 participants

@astrofrog @perrygreenfield @pllim @Cadair