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

sample implementation of a (uncorrelated) combined fit #134

Merged
merged 13 commits into from
Feb 2, 2023

Conversation

PiaLJP
Copy link
Contributor

@PiaLJP PiaLJP commented Dec 16, 2022

A combined fit is performed when dictionaries (instead of lists) are handed over to the fits.least_squares function.

@fjosw
Copy link
Owner

fjosw commented Dec 16, 2022

Hi Pia, it looks like your PR breaks a few tests. Please have a look at CONTRIBUTING.md where I tried to explain how to run the tests on a local machine.

@s-kuberski
Copy link
Collaborator

Thanks for the implementation. Before I inspect everything in detail, I'd like to ask a general question: Is it really necessary to do the combined fit in a new routine that has many common aspects with _standard_fit? I have the feeling that this will increase the amount of work that will be needed in the future to implement new routines and to fix and avoid bugs.

My first attempt would have been to write a wrapper around _standard_fit or to adapt the first lines of _standard_fit such that proper combinations x, y, func are handed to the routine. For x and y this is trivial, for func one could just settle on a list of functions that is filled with the same function over and over again if no combined fit is done. Did you look at this and decided to not to do this for some reason (performance?)?

For the future of the fit routines in pyerrors, I'd rather merge all the fit routines (i.e., also _prior_fit would be considered here) instead of adding new routines.

@fjosw
Copy link
Owner

fjosw commented Jan 3, 2023

I agree that this is not a good solution and results in a lot of unnecessary code duplication and maintainability problems. I see this PR as a step towards a new version of the fit module as it already defines the interface for combined fits and provides test cases which will also have to be fulfilled by the final version which could be very helpful in the next development phase.

In this spirit it would be very interesting to hear your opinion on the interface @PiaLJP designed and the overall functionality, not so much the actual implementation. As soon as we agree on the interface we could then start to rewrite and refactor the code.

@fjosw fjosw added the enhancement New feature or request label Jan 3, 2023
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 very much for your effort. Let's go with the dictionary solution by now. In contrast to the other solutions that I had in mind, it would remove the need to have x, y and func in the same order, if the implementation is adapted accordingly.

pyerrors/fits.py Outdated Show resolved Hide resolved
pyerrors/fits.py Outdated Show resolved Hide resolved
pyerrors/fits.py Outdated Show resolved Hide resolved
pyerrors/fits.py Outdated Show resolved Hide resolved
pyerrors/fits.py Outdated Show resolved Hide resolved
pyerrors/fits.py Outdated Show resolved Hide resolved
pyerrors/fits.py Outdated Show resolved Hide resolved
pyerrors/fits.py Outdated Show resolved Hide resolved
tests/fits_test.py Show resolved Hide resolved
…c input dictionaries are not in the same order, build: improvements in performance
pyerrors/fits.py Outdated Show resolved Hide resolved
pyerrors/fits.py Outdated Show resolved Hide resolved
@s-kuberski
Copy link
Collaborator

Hi. Thanks for adapting the code. I have made one minor comment that we could resolve before merging.

Co-authored-by: Simon Kuberski <simon.kuberski@uni-muenster.de>
@fjosw fjosw merged commit 3236ba5 into fjosw:develop Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants