Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Joint fitting bug fix#20074

Open
KriSun95 wants to merge 11 commits into
astropy:mainastropy/astropy:mainfrom
KriSun95:joint-fitting-bug-fixKriSun95/astropy:joint-fitting-bug-fixCopy head branch name to clipboard
Open

Joint fitting bug fix#20074
KriSun95 wants to merge 11 commits into
astropy:mainastropy/astropy:mainfrom
KriSun95:joint-fitting-bug-fixKriSun95/astropy:joint-fitting-bug-fixCopy head branch name to clipboard

Conversation

@KriSun95

@KriSun95 KriSun95 commented Jul 15, 2026

Copy link
Copy Markdown

This fix #20073

Instead of deleting the elements in params one by one while looping, I've elected to obtain and store all the indices to be deleted first and delete them in the one go. This means the model_to_fit_params method now extracts the correct and corresponding initial values for the fittable parameters from the models.

I opted for the numpy.delete method instead of the "loop through the reverse of the list" approach the other fitters use since the user could technically give the joint parameters in any order when initialising JointFitter. If the user doesn't give the joint parameters in the correct order this would also throw off the correct indexing.

P.S.: I have a new more consistent Astropy fitter API JointFitter class that I aim to open a PR about in the coming weeks that includes very minimal edits to the fitting module outside of the JointFitter class itself (this new class also addresses Issue #8814). I just wanted to open this PR and Issue since I spotted a bug that should probably be fixed regardless.

@KriSun95
KriSun95 requested a review from a team as a code owner July 15, 2026 20:25
@github-actions

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 commented Jul 16, 2026

Copy link
Copy Markdown
Member

Hello and thanks.

If this is a bug, then please add a change log; see https://github.com/astropy/astropy/blob/main/docs/changes/README.rst

Also would need a regression test demonstrating that this bug would cause a failure on main but not with your patch.

@KriSun95

KriSun95 commented Jul 16, 2026

Copy link
Copy Markdown
Author

I've added a changelog and a test showing the bug and then the patch fixing it.

What I added to test_fitters.TestJointFitter:

  1. The JointFitter edits the model parameter values in place rather than returning a copy of the models altered; therefore, I had to initialise another couple of models (self.g3=models.Gaussian1D(amplitude=10, mean=14.9, stddev=0.3) and self.g4=models.Gaussian1D(amplitude=10, mean=13, stddev=0.4)) to pass to the new JointFitter in TestJointFitter().setup_class().

  2. I then added a new JointFitter instance called self.jf2 (since the first one is called to fit straight away and to preserve older test behaviour), also in TestJointFitter().setup_class().

  3. Finally, I created a new method called test_initial_parameter_values that will check the values JointFitter uses as the initial values for all of the parameters to be fitted.

The two behaviours are:

  1. In main, test_initial_parameter_values fails as the initial values from self.fitparams should be [ 9.8, 14.0, 0.3, 0.4] but instead gets[9.8, 14.0, 14.9, 13.0] as the bug has been described.

  2. The patch I've suggest here passes this test and all other existing TestJointFitter tests.

@KriSun95

Copy link
Copy Markdown
Author

Apologies @pllim , I forgot to tag you once I had attempting to take your recommendations into account. Please let me know if there is anything else required in the PR or if the test I've added is not adequate and needs improved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

JointFitter obtains incorrect initial values for fitting

2 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.