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

Rewrite get_pe_res: numerical FWHM calculation #137

Merged
merged 3 commits into from
Mar 19, 2025

Conversation

benedikt-nagler
Copy link
Contributor

@benedikt-nagler benedikt-nagler commented Mar 18, 2025

Fix #130.

The previous implementation of the get_pe_res function was incorrect. Since there is no closed formula for the FWHM of a gaussian mixture, it is now calculated numerically.

For each given subset of the gaussian mixture model, the maximum of the resulting PDF is calculated by dividing the interval [0.8, 1.2] into small steps. The step size depends on the largest variance of the individual gaussians, which ensures a high enough accuracy for the estimated maximum.

After that, all values where the half maximum is reached are calculated with the find_zeros function from Roots.jl. The FWHM then corresponds to the difference between the largest and the smallest position.

@benedikt-nagler benedikt-nagler linked an issue Mar 18, 2025 that may be closed by this pull request
Copy link

codecov bot commented Mar 18, 2025

Codecov Report

Attention: Patch coverage is 0% with 14 lines in your changes missing coverage. Please review.

Project coverage is 28.24%. Comparing base (4a43dfa) to head (cb331e9).
Report is 16 commits behind head on main.

Files with missing lines Patch % Lines
src/sipmfit.jl 0.00% 14 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #137      +/-   ##
==========================================
- Coverage   28.39%   28.24%   -0.16%     
==========================================
  Files          37       37              
  Lines        3564     3583      +19     
==========================================
  Hits         1012     1012              
- Misses       2552     2571      +19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@fhagemann fhagemann changed the title Rewrite get_pe_res: numerical FWHM calculation Fix #130 Rewrite get_pe_res: numerical FWHM calculation Mar 18, 2025
@fhagemann
Copy link
Contributor

Unit tests?

@theHenks theHenks requested a review from fhagemann March 18, 2025 17:04
@theHenks theHenks added bug Something isn't working breaking Breaking change labels Mar 18, 2025
Copy link
Contributor

@fhagemann fhagemann left a comment

Choose a reason for hiding this comment

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

Manchmal sieht man den Wald vor lauter Bäumen nicht 🥠🌳🔥

@theHenks
Copy link
Collaborator

I am happy with the PR. Let's finish this soon and merge!
Thanks a lot @benedikt-nagler 😍

@benedikt-nagler
Copy link
Contributor Author

I am happy with the PR. Let's finish this soon and merge! Thanks a lot @benedikt-nagler 😍

Glad I could help!

Copy link
Contributor

@fhagemann fhagemann left a comment

Choose a reason for hiding this comment

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

If this works without throwing an error, I would also be happy to merge.

@theHenks
Copy link
Collaborator

If this works without throwing an error, I would also be happy to merge.

I take this as a yes.

@theHenks theHenks merged commit ce81f77 into legend-exp:main Mar 19, 2025
10 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Determine resolution of P.E. peaks in SiPM gaussian mixture
3 participants