Repository navigation
Fix Polynomial2D fitting when used in compound model - #17618
Conversation
…els preserve input shape while some don't when returning fit_deriv
|
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.
|
|
👋 Thank you for your draft pull request! Do you know that you can use |
|
Ah I found the remaining issue, however I will need to wait until later today or tomorrow to fix. Please hold... |
…shape gets transposed when it shouldn't
|
I think this should now be fixed |
a5bcfc9 to
42a46e4
Compare
perrygreenfield
left a comment
perrygreenfield
left a comment
There was a problem hiding this comment.
LGTM except for a comment on the last test.
| p_init = Polynomial2D(degree=2) + Gaussian2D(amplitude=50000, x_mean=60, y_mean=60) | ||
| fit_p = DogBoxLSQFitter() | ||
|
|
||
| p = fit_p(p_init, x, y, z) |
There was a problem hiding this comment.
I'm puzzled at the intent of this test. Is it to ensure no exception happens for the fit? Nothing seems to be done with the fit results.
There was a problem hiding this comment.
Yes it's just a regression test for this bug where the fitting raised an exception
There was a problem hiding this comment.
A comment to that effect would clarify that.
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
|
|
||
| @pytest.mark.skipif(not HAS_SCIPY, reason="requires scipy") | ||
| def test_fit_compound_polynomial2d(): | ||
| # Regression test for a bug that caused compound models with Polynomial2D |
There was a problem hiding this comment.
Could you just make this a docstring?
…618-on-v7.0.x Backport PR #17618 on branch v7.0.x (Fix Polynomial2D fitting when used in compound model)
4 participants
This fixes #17509
The issue was that Polynomial2D 'forgets' about the dimensionality of the input and so always returns a 2D array, whereas other models don't. I really think we should at some point tighten up the fit_deriv methods to be more consistent, but since that usage has been allowed for now, I am fixing our implementation of
CompoundModel.fit_derivto be more robust.I have expanded the tests compared to the numerical derivatives and while Polynomial2D works, in fact some other models are failing when passing 2D inputs so I am going to try and see how to fix that.