-
-
Notifications
You must be signed in to change notification settings - Fork 140
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove LevMarLSQFitter #1899
Remove LevMarLSQFitter #1899
Conversation
@@ -105,11 +105,13 @@ def centroid_1dg(data, error=None, mask=None): | |||
|
|||
xy_data = [np.ma.sum(data, axis=i).data for i in (0, 1)] | |||
|
|||
# would need to use a different fitter if parameter bounds are used | |||
fitter = LMLSQFitter() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@larrybradley - gaussians have bounds enabled by default (on stddev) so if you are fitting a Gaussian I think it would be best to use TRFLSQFitter or another one, not LMLSQFitter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@astrofrog Are you sure about that? Where are the bounds defined? I don't see any bounds defined in functional_models
? Do you mean constrained to be postive? I still get negative stddev's sometimes.....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've also noticed that TRFLSQFitter
is about 4-5% slower than LevMarLSQFitter
even without any bounds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@larrybradley yes the fact the stddev is required to be positive - it's formally treated as a bound.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you see negative stddev values with LMLSQ or TRFLSQ or both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've also noticed that
TRFLSQFitter
is about 4-5% slower thanLevMarLSQFitter
even without any bounds.
Luckily the fitters will be significantly faster with astropy 7.0.0 so should be more than enough to compensate for this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see that's only for Gaussian1D
. I was seeing negative stddev when fitting Gaussian2D
. Why is Gaussian1D
stddev bounded, but not Gaussian2D
? Should I open a PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR changes the default Astropy fitter for
PSFPhotometry
,IterativePSFPhotometry
, andEPSFFitter
fromLevMarLSQFitter
toTRFLSQFitter
.LevMarLSQFitter
uses the Levenberg-Marquardt algorithm via the SciPy legacy functionscipy.optimize.leastsq
, which is no longer recommended. This fitter supports parameter bounds using an unsophisticated min/max condition where parameters that are out of bounds are simply reset to the min or max of the bounds during each step. This can cause parameters to stick to one of the bounds during the fitting process if the parameter gets close to the bound. If needed, this fitter can still be used by explicitly setting the fitter in thePSFPhotometry
,IterativePSFPhotometry
, andEPSFFitter
classes.The fitter used in
RadialProfile
to fit the profile with a Gaussian was also changed fromLevMarLSQFitter
toTRFLSQFitter
.The fitter used in
centroid_1dg
andcentroid_2dg
was changed fromLevMarLSQFitter
toLMLSQFitter
.See astropy/astropy#16983 for more information.
Closes: #1895