Add fitting of afterpulse spectrum to derive single pe application. #1446
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds the capability to fit the afterpulse spectrum as part of the single photon electron spectrum derivation. Key changes include:
- Integration of an exponential decay fit with an optional fixed decay factor parameter.
- Updates to unit tests to support the new parameters and function signatures.
- Improvements in command-line configuration and output path resolution for the derived spectra.
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/unit_tests/camera/test_single_photon_electron_spectrum.py | Updated test cases to pass the new parameters required by the updated spectrum derivation functions. |
| src/simtools/camera/single_photon_electron_spectrum.py | Modified key functions (_derive_spectrum_norm_spe, _get_input_data, fit_afterpulse_spectrum, afterpulse_fit_function, etc.) to support new afterpulse fitting features. |
| src/simtools/applications/derive_photon_electron_spectrum.py | Updated command-line arguments to reflect the new fitting options and amplitude range for afterpulse analysis. |
| src/simtools/applications/run_application.py | Adjusted output path construction and list comprehension to correctly handle string replacements in configuration. |
Files not reviewed (1)
- tests/resources/LST_afterpulses.ecsv: Language not supported
Comments suppressed due to low confidence (1)
src/simtools/camera/single_photon_electron_spectrum.py:244
- [nitpick] In the docstring for afterpulse_fit_function, the parameter is referred to as 'fix_K' instead of 'fix_k'. Consider updating the docstring for parameter naming consistency.
def afterpulse_fit_function(self, fix_k):
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| "--threshold_afterpulse_spectrum", | ||
| help="Threshold for afterpulse ratio calculation", | ||
| "--afterpulse_amplitude_range", | ||
| help="Amplitude range in pe for afterpulse calculation", |
There was a problem hiding this comment.
Is this range for calculating the afterpulse? (we don't actually calculate it.)
Perhaps it is the range for fitting? In the other PR the limit from below was defaulted to 4.0 which makes more sense to me.
There was a problem hiding this comment.
- fit is done in the range of available data points (so no user input on this)
afterpulse_amplitude_rangeis the range over which the fitted function is used to calculate afterpulses and forward it to norm_spe
Any suggestion on how to name it better? I don't have a good idea...
There was a problem hiding this comment.
Name is fine (or it could be afterpulse_spectrum_range if you want). Perhaps the point is about the word "calculation", because I don't expect we calculate the afterpulsing spectrum. Either way, it's not a big deal, you can leave it as is as well.
tests/integration_tests/config/derive_photon_electron_spectrum_lst_ecsv.yml
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
Analysis Details0 Issues
Coverage and Duplications
Project ID: gammasim_simtools_AY_ssha9WiFxsX-2oy_w |
Review after merging of #1419
Add option to fit afterpulse spectrum to the derive single pe application. Fitted is a exponential function with linear decay term. Decay factor (
k) can be fixed; this might be necessary if measurements of afterpulses do not reach large single pe amplitudes,New configuration parameters for
derive_photon_electron_spectrum.py:fit_afterpulse- turn on afterpulse fittingfit_afterpulse_decay_factor- this will fix the decay factorkto the given value (for LSTN, 15 is a good value)afterpulse_amplitude_range- fit function will be used in this range to predict afterpulse amplitudes forwarded tonorm_speThe integration test in
tests/integration_tests/config/derive_photon_electron_spectrum_lst_ecsv.ymlhas been adopted and applies a fit now. Updated test files to include errors on afterpulse measurements.There is a bit of fine tuning for LSTN right now; expect that some smaller changes will be necessary when using other data.
Added also a fix in run_applications handling Paths/str for the output path naming.