-
Notifications
You must be signed in to change notification settings - Fork 101
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 GLSFitter
dependence from Residuals
#1618
Conversation
GLSFitter
dependence from Residuals
GLSFitter
dependence from Residuals
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1618 +/- ##
=======================================
Coverage 68.24% 68.24%
=======================================
Files 99 99
Lines 22786 22806 +20
Branches 3918 3920 +2
=======================================
+ Hits 15551 15565 +14
- Misses 6274 6278 +4
- Partials 961 963 +2
☔ View full report in Codecov by Sentry. |
GLSFitter
dependence from Residuals
GLSFitter
dependence from Residuals
I assume there is no big performance hit when using this? Or maybe it's even faster? |
It's the same code that was in GLSFitter, sans a few unnecessary lines. There is no reason it should be slower. |
I agree that there is no reason. and it probably does less copying of objects. but I just wanted to verify empirically. |
I did the profiling. The old code takes about 4 s whereas the new code takes about 1 s. |
great! that's what I was hoping. |
There is an API change in this that has to be documented. |
Is this ready to merge? |
I guess on the PINT call last week we were discussing whether or not this would lead to changes in any example notebooks. Have you looked to see what needs to be done? |
I checked. The change doesn't affect any notebooks. I have updated the docstrings to reflect the API change. |
Yes. |
But the
presumably this is trying to construct residuals and just doing a fit because the API required it? |
I'll check this. |
I don't think those commented out lines are needed. I have removed them. The API change is only in Residuals and not GLSFitter. The understanding_fitters notebook does not use Residuals directly at all. So I don't think we need any other change in this file. |
This requires a bit of code duplication, but I think it is worth decoupling these modules. Also, this avoids a bunch of unnecessary fitting steps while computing the chisq.