Joint fitting bug fix#20074
Joint fitting bug fix#20074KriSun95 wants to merge 11 commits intoastropy:mainastropy/astropy:mainfrom KriSun95:joint-fitting-bug-fixKriSun95/astropy:joint-fitting-bug-fixCopy head branch name to clipboard
Conversation
|
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.
|
|
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 |
|
I've added a changelog and a test showing the bug and then the patch fixing it. What I added to
The two behaviours are:
|
|
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. |
This fix #20073
Instead of deleting the elements in
paramsone 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 themodel_to_fit_paramsmethod now extracts the correct and corresponding initial values for the fittable parameters from the models.I opted for the
numpy.deletemethod 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 initialisingJointFitter. 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
JointFitterclass that I aim to open a PR about in the coming weeks that includes very minimal edits to thefittingmodule outside of theJointFitterclass 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.