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

SPE fit algorithm: account for fit convergence to set valid status #106

Closed
tibaldo opened this issue Feb 5, 2024 · 4 comments · Fixed by #108
Closed

SPE fit algorithm: account for fit convergence to set valid status #106

tibaldo opened this issue Feb 5, 2024 · 4 comments · Fixed by #108
Labels
enhancement New feature or request

Comments

@tibaldo
Copy link
Member

tibaldo commented Feb 5, 2024

While looking at he new implementation of the gain calibration via SPE fitting using run 3936, I found that for two pixels I got poor results with the fitted function not matching the data
fit_SPE_pixel658
fit_SPE_pixel660
Yet, the gain value is not flagged in the results file, that is, it has 'is_valid = True'.
By looking at the code it seems that presently the only condition to set 'is_valid = True' is that the fit algorithm returns some values. I think the fit convergence should be also taken into account to avoid this kind of problem.
I explored this is my branch tibaldo:spefit_convergence_issues and it seems rather straighforward. In this exemple of a possible implementation I checked three outputs from Minuit: fit convergence status, validity of parameters and that the parameters have not reached their limits.
What do you think @guillaumegrolleron @jlenain? If that seems like a good addition I can make any modifications needed and then make a pull request from my branch.
PS: I did not dig into the origin of the problem for the two pixels. For pixel 658 there is a failure in the preliminary Gaussian fits to set the starting value for the parameters, for pixel 660 the charge distribution looks unusual, but I'm unsure if it is just a plot range feature.

@tibaldo tibaldo added the enhancement New feature or request label Feb 5, 2024
@jlenain
Copy link
Collaborator

jlenain commented Feb 8, 2024

Hi @tibaldo , sorry for the delay and thanks for this!
Indeed, it would be very useful to add some criteria to flag fit results as valid. The ones you implemented in https://github.com/tibaldo/nectarchain/tree/spefit_convergence_issues seem fully valid, so yes, please go ahead with a pull request, that would be much appreciated!

@tibaldo
Copy link
Member Author

tibaldo commented Feb 8, 2024

Excellent, thank you. I'll make sure this is covered in the unit tests and make apull request probably tomorrow.

@jlenain
Copy link
Collaborator

jlenain commented Feb 8, 2024

So, discussing with @guillaumegrolleron , it seems he is already implementing these changes while working on #89 - where he is working on forcing the spawn implementation on Linux as well (which would then also be future-proof). So maybe better to coordinate with @guillaumegrolleron for such a pull request. Many thanks!

@tibaldo
Copy link
Member Author

tibaldo commented Feb 8, 2024

@guillaumegrolleron let me know how you want to proceeds, anything works for me

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 a pull request may close this issue.

2 participants