-
Notifications
You must be signed in to change notification settings - Fork 76
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Use TRFLSQFitter instead of LevMarLSQFitter #3202
Conversation
as per Astropy recommendation
@@ -876,7 +876,7 @@ def calculate_photometry(self, dataset=None, aperture=None, background=None, | |||
|
|||
# Fit Gaussian1D to radial profile data. | |||
if self.fit_radial_profile: | |||
fitter = LevMarLSQFitter() | |||
fitter = TRFLSQFitter() |
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 , is this replacement reasonable or another one is better? Also see astropy/photutils#1895
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.
That should work fine. I don't think you are using parameter bounds, so LMLSQFitter
is also an option.
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.
Thanks! Well, not yet. No idea if that is going to be future request or not. 😬
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.
is there any consequence on performance between the two for this use-case?
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.
Nothing that we have to worry about. I am using what Astropy now suggests in their docs. Tom R made that change upstream and he has been also working on modeling performance there, so he wouldn't recommend anything that would make things worse.
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.
In my testing, TRFLSQFitter
is about 4-5% slower than LevMarLSQFitter
. However, this difference will not be noticeable here.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3202 +/- ##
=======================================
Coverage 88.50% 88.50%
=======================================
Files 124 124
Lines 18577 18577
=======================================
Hits 16441 16441
Misses 2136 2136 ☔ View full report in Codecov by Sentry. |
Thanks for the review! |
Description
This pull request is to fix #3189
Change log entry
CHANGES.rst
? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rst
before merge. If no, maintainershould add a
no-changelog-entry-needed
label.Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
trivial
label.