-
-
Notifications
You must be signed in to change notification settings - Fork 129
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
Replace LevMarLSQFitter with TRFLSQFitter #1180
Conversation
because we are using a different fitter now.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1180 +/- ##
=======================================
Coverage 86.86% 86.86%
=======================================
Files 63 63
Lines 4553 4553
=======================================
Hits 3955 3955
Misses 598 598 ☔ View full report in Codecov by Sentry. |
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.
lgtm. Could you confirm that those numerical changes were caused by switching default fitter?
How should I do that? That is the only change and then the numbers change. Not sure what else I can do. Hope you can advise. Thanks. |
That's pretty much all I was asking. I don't see any other differences. I would like to know what these changes imply about the absolute accuracy of the two methods, or perhaps about their default tolerances, but that desire shouldn't hold back merging this. |
@astrofrog did astropy/astropy#16983 so maybe he can explain. I am just propagating that recommendation downstream. 🙏 |
The differences here seem pretty small considering it is a completely different fitting algorithm. Note that TRFLSQFitter does have |
I'll leave that exercise for the future but I am uncertain who will do it. 😏 Thanks, all! Merging. |
Related PRs:
TRFLSQFitter
also hascalc_uncertainties
and should respect bounds.Fix #1172