Bug fix and enhancement of pedestal tool plus scripts to analyse thermal tests data #230
Conversation
Use online mean/variance algorithm for efficiency
…ltiple subsamples
… combined results clearer
…ry#213) * Only fill charges container if waveforms are available * Cleanup commented stuff from some previous PR * Pin `persistent` (dependency for `ZODB`) version to fix CI --------- Co-authored-by: Jean-Philippe Lenain <[email protected]> # Conflicts: # src/nectarchain/makers/component/pedestal_component.py
…ry#213) * Only fill charges container if waveforms are available * Cleanup commented stuff from some previous PR * Pin `persistent` (dependency for `ZODB`) version to fix CI --------- Co-authored-by: Jean-Philippe Lenain <[email protected]> # Conflicts: # src/nectarchain/makers/component/pedestal_component.py
…ry#213) * Only fill charges container if waveforms are available * Cleanup commented stuff from some previous PR * Pin `persistent` (dependency for `ZODB`) version to fix CI --------- Co-authored-by: Jean-Philippe Lenain <[email protected]> # Conflicts: # src/nectarchain/makers/component/pedestal_component.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #230 +/- ##
==========================================
- Coverage 51.79% 51.68% -0.11%
==========================================
Files 78 78
Lines 6505 6598 +93
==========================================
+ Hits 3369 3410 +41
- Misses 3136 3188 +52 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Hi @tibaldo !
Thanks a lot for this work !
This PR looks good to me. I just made a few, minor modifications after commenting some parts.
Do you intend to push further commits to this PR, or could we un-draft and merge it ?
One comment is pending, though, [EDIT]: and I also would like to push a final commit to fix the docs in CI.
| else: | ||
| log.error("Unexpected shape for means array") | ||
|
|
||
| mean = np.sum(nevents_expanded * means, axis=0) / total_nevents_expanded |
There was a problem hiding this comment.
Hi @tibaldo !
I was wondering whether this static method is really needed. To my understanding, numpy already offers the possibility to compute the mean and standard deviations when input with several arrays, see e.g. https://stackoverflow.com/a/18461943. I think the same should apply to numpy.std.
There was a problem hiding this comment.
Hi @jlenain, thanks a lot for reviewing the PR and for the fixes you made in several places. I'm going to undraft it so that when we have solved this pending item it can be merged.
The method mean_std_multisample calculates the mean and standard deviation of a combined sample from the aggregated statistics (means and standard deviations) of multiple subsamples. To my knowledge numpy.mean and numpy.std always operate directly on raw data (it is the case in the stack overflow thread you linked if I understand it correctly) — they can’t combine aggregated statistics.
If you know a numpy function that calculates the mean and standard deviation of a combined sample from the aggregated statistics of multiple subsamples could you please point me to it?
2 participants
This PR combined several fixes and enhancements related to pedestal extraction and assessement in thermal tests data.
PedestalNectarCAMCalibrationToolin the computation of the the pedestal RMS for a combined dataset when using slicing.NectarCAMPedestalContainer,PedestalEstimationComponent, andPedestalNectarCAMCalibrationToolto systematically compute statistics for the pedestal charge (useful for performance assessment and monitoring).plot_pedestal_output.py) due to evolutions in the naming conventions within nectarchain.