Skip to content
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

refactor: _standard_fit method made redundant. #154

Merged
merged 6 commits into from Mar 1, 2023
Merged

Conversation

fjosw
Copy link
Owner

@fjosw fjosw commented Feb 23, 2023

I started refactoring the fit code and removed the _standard_fit function by merging its functionality with _combined_fit. All tests pass so it looks like nothing went wrong in the process but please have a close look @s-kuberski & @PiaLJP .

Copy link
Collaborator

@s-kuberski s-kuberski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the changes. I spotted a bug that is not caught by the tests.

pyerrors/fits.py Outdated Show resolved Hide resolved
@s-kuberski
Copy link
Collaborator

Thanks! The two functions residual_plot and qqplot are invalid for correlated fits. This has been the case before and I don't say that we need to resolve this issue now. If, at some point, we want to show the transformed residuals that enter a correlated fit, we would have to supply chol_inv and the complete x and y vectors, right?

@fjosw
Copy link
Owner Author

fjosw commented Feb 28, 2023

I would say that residual_plot is not invalid, one just would not expect that the residuals are distributed ~ N(0, 1) if there are correlations between the data points. I think one could transform the residuals, but I am unsure if the transformation and the resulting visual representation are unambiguous. I would propose leaving it as it is for now.

@s-kuberski
Copy link
Collaborator

I agree. We can keep this in mind for the future.

@PiaLJP
Copy link
Contributor

PiaLJP commented Mar 1, 2023

Maybe it would be a good idea to add a warning then, along the lines of 'For correlated data points the residuals are not necessarily expected to be ~N(0,1)'? Apart from that I also think it looks good now, thanks.

@fjosw fjosw merged commit dc7033e into develop Mar 1, 2023
@fjosw fjosw deleted the refactor/fits branch March 1, 2023 10:03
@s-kuberski s-kuberski mentioned this pull request Mar 3, 2023
@fjosw fjosw mentioned this pull request Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants