Allow for different thresholds for OpHitFinder by asanchezcastillo · Pull Request #36 · LArSoft/larana

asanchezcastillo · 2025-01-22T17:10:09Z

SBND optical reconstruction requires using individual threshold for each channel. This PR incorporates the required changes which are summarized below:

Setting an individual individual threshold for each channel requires carrying the channel number information until the specific reconstruction algorithm. Even thought RunHitFinder takes a raw:OpDetWaveform this is later converted to a pmtana::Waveform_t on the Reconstruct method of PulseRecoManager losing the required information about the channel number. The same happens for the Reconstruct method of PMTPulseRecoBase and the RecoPulse method implemented by the reconstruction algorithm. Fortunately enough, both pmtana::Waveform_t and raw::OpDetWaveform inherit from std::vector<> which to the best of my knowledge makes backwards compatibility given just by changing the object we pass into the former methods.
Change the relevant peak finder algorithms to allow for individual threshold setting. The lists of threshold per channel must be passed through a fhicl parameter.

…algorithms

Update dependencies

FNALbuild · 2025-01-22T17:10:37Z

A new Pull Request was created by @asanchezcastillo (Alejandro Sánchez Castillo) for develop.

It involves the following packages:

larana

@LArSoft/level-1-managers, @LArSoft/level-2-managers can you please review it and eventually sign? Thanks.

cms-bot commands are listed here

FNALbuild · 2025-01-22T17:10:38Z

The code-checks are being triggered in jenkins.

FNALbuild · 2025-01-22T17:10:49Z

-code-checks
Pull request failed code-formatting checks. Please ensure that cetmodules has been setup and execute the following command from the top-level directory of your repository:

format-code \
  larana/OpticalDetector/OpHitFinder/AlgoCFD.cxx \ 
  larana/OpticalDetector/OpHitFinder/AlgoSiPM.cxx \ 
  larana/OpticalDetector/OpHitFinder/AlgoSlidingWindow.cxx

Then commit the changes and push them to your PR branch.

FNALbuild · 2025-01-27T15:00:30Z

Pull request #36 was updated. @LArSoft/level-1-managers, @LArSoft/level-2-managers can you please check and sign again.

FNALbuild · 2025-01-27T15:00:31Z

The code-checks are being triggered in jenkins.

FNALbuild · 2025-01-27T15:00:43Z

+code-checks

knoepfel · 2025-01-27T16:21:20Z

@asanchezcastillo, do you intend this PR to be in draft state?

asanchezcastillo · 2025-01-27T16:26:22Z

@knoepfel I am marking it as ready for review now. Thanks for asking.

absolution1 · 2025-02-04T22:54:28Z

trigger build

FNALbuild · 2025-02-04T22:54:53Z

The tests are being triggered in jenkins.

FNALbuild · 2025-02-04T23:06:26Z

+LArSoft tests OK on slf7 for c14:prof
for details see
https://lar-ci-history.fnal.gov/LarCI/app/ns:LArSoft/view_builds/index?offset=0&builds=lar_ci/24905&builds=
for details of the parent CI build see
https://lar-ci-history.fnal.gov/LarCI/app/view_builds/index?offset=0&builds=&builds=

FNALbuild · 2025-02-05T03:18:18Z

-DUNE tests failed on slf7 for e26:prof
for details see
https://lar-ci-history.fnal.gov/LarCI/app/ns:DUNE/view_builds/index?offset=0&builds=dune_ci/18740&builds=
for details of the parent CI build see
https://lar-ci-history.fnal.gov/LarCI/app/view_builds/index?offset=0&builds=lar_ci/24910&builds=

absolution1

Thank you for the PR. I've added a set of comments about removing the default fcl values.

absolution1 · 2025-02-05T10:46:36Z

larana/OpticalDetector/OpHitFinder/AlgoCFD.cxx

+    _peak_thresh_by_channel = pset.get<bool>("ThreshByChannel", false);
+    _peak_thresh_vector = pset.get<std::vector<double>>("PeakThreshVector", {});


It looks like this module is not using fcl validation, so default fcl values should be avoided. Could these be removed from the producer, and then add the parameters to the default fcl?

We suggest a different approach. It looks like two configuration parameters are being used: one that says "use the threshold-by-channel option", and another that says "here are the thresholds by channel". Instead of using two parameters, it is better to use one optional parameter. For example, make this change:

- //Whether to apply an individual threshold for each channel - bool _peak_thresh_by_channel; - //Vector of thresholds for each channel - std::vector<double> _peak_thresh_vector; + //Optional user-specified vector of thresholds for each channel + std::optional<std::vector<double>> _peak_thresh_vector;

You would then switch to:

_peak_thresh_vector = pset.get_if_present<std::vector<double>>("PeakThreshVector");

If _peak_thresh_vector is "engaged" (i.e. there's a value associated with it), then "PeakThreshVector" has been specified in the configuration. If it is not engaged, then there is no configuration parameter present. The check would then look something like:

if (_peak_thresh_vector) { uint ChannelNumber = wf.ChannelNumber(); if (ChannelNumber >= _peak_thresh_vector->size()) throw cet::exception("OpHitFinder") << "Threshold not found for channel " << ChannelNumber << "\n"; _peak_thresh = (*_peak_thresh_vector)[ChannelNumber]; }

absolution1 · 2025-02-05T10:46:54Z

larana/OpticalDetector/OpHitFinder/AlgoSiPM.cxx

    : PMTPulseRecoBase(name)
  {
-
+    _adc_thres_by_channel = pset.get<bool>("ADCThresholdByChannel", false);


same comment about default values

absolution1 · 2025-02-05T10:47:08Z

larana/OpticalDetector/OpHitFinder/AlgoSiPM.cxx

  {
-
+    _adc_thres_by_channel = pset.get<bool>("ADCThresholdByChannel", false);
+    _adc_thres_vector = pset.get<std::vector<float>>("ADCThresVector", {});


same comment about default values

absolution1 · 2025-02-05T10:47:44Z

larana/OpticalDetector/OpHitFinder/AlgoSlidingWindow.cxx

+    _adc_thres_by_channel = pset.get<bool>("ADCThresholdByChannel", false);
+
+    _adc_thres_vector = pset.get<std::vector<float>>("ADCThresVector", {});
+


default values

FNALbuild · 2025-02-06T09:50:00Z

Pull request #36 was updated. @LArSoft/level-1-managers, @LArSoft/level-2-managers can you please check and sign again.

FNALbuild · 2025-02-06T09:50:00Z

The code-checks are being triggered in jenkins.

FNALbuild · 2025-02-06T09:50:11Z

+code-checks

knoepfel

@asanchezcastillo, thanks for this PR. The switch from Waveform_t to OpDetWaveform is appropriate. See the comment below regarding configuration. Instead of doing everything with two parameters (which introduces an awkwardness for the user), we suggest doing it with one where no defaults are required.

knoepfel · 2025-02-11T17:42:23Z

larana/OpticalDetector/OpHitFinder/AlgoCFD.cxx

+    _peak_thresh_by_channel = pset.get<bool>("ThreshByChannel", false);
+    _peak_thresh_vector = pset.get<std::vector<double>>("PeakThreshVector", {});


We suggest a different approach. It looks like two configuration parameters are being used: one that says "use the threshold-by-channel option", and another that says "here are the thresholds by channel". Instead of using two parameters, it is better to use one optional parameter. For example, make this change:

- //Whether to apply an individual threshold for each channel - bool _peak_thresh_by_channel; - //Vector of thresholds for each channel - std::vector<double> _peak_thresh_vector; + //Optional user-specified vector of thresholds for each channel + std::optional<std::vector<double>> _peak_thresh_vector;

You would then switch to:

_peak_thresh_vector = pset.get_if_present<std::vector<double>>("PeakThreshVector");

If _peak_thresh_vector is "engaged" (i.e. there's a value associated with it), then "PeakThreshVector" has been specified in the configuration. If it is not engaged, then there is no configuration parameter present. The check would then look something like:

if (_peak_thresh_vector) { uint ChannelNumber = wf.ChannelNumber(); if (ChannelNumber >= _peak_thresh_vector->size()) throw cet::exception("OpHitFinder") << "Threshold not found for channel " << ChannelNumber << "\n"; _peak_thresh = (*_peak_thresh_vector)[ChannelNumber]; }

knoepfel · 2025-02-24T16:37:42Z

@asanchezcastillo, are you able to implement the suggested changes above?

Update to larana v10_00_09

FNALbuild · 2025-02-26T10:36:32Z

Pull request #36 was updated. @LArSoft/level-1-managers, @LArSoft/level-2-managers can you please check and sign again.

FNALbuild · 2025-02-26T10:36:32Z

The code-checks are being triggered in jenkins.

FNALbuild · 2025-02-26T10:36:44Z

+code-checks

asanchezcastillo · 2025-02-27T11:10:36Z

Hi @knoepfel. There was a build issue with duneopdet that we are currently tying to fix. I do not think it will take long though, so when we are done I will incorporate the changes into the PR. Thanks!

lgarren · 2025-03-10T15:27:15Z

@asanchezcastillo Do you have time to update this PR now?

knoepfel · 2025-05-19T15:18:35Z

@asanchezcastillo, we have converted this PR to a draft until DUNE/duneopdet#83 is ready.

asanchezcastillo and others added 4 commits January 17, 2025 07:59

Change type pmtana::Waveform_t to raw::OpDetWaveform for OpHitFinder …

d09f49d

…algorithms

larana v09_15_15 for larsoft v09_93_01

885fe73

Add individual channel threshold for ophitfinding algorithms

4a8cca3

Merge pull request #1 from LArSoft/develop

3d534da

Update dependencies

FNALbuild added L1-pending code-checks-pending L2-pending signatures-pending tests-pending labels Jan 22, 2025

FNALbuild added code-checks-rejected and removed code-checks-pending labels Jan 22, 2025

Format code

9d845cc

FNALbuild added code-checks-pending and removed code-checks-rejected labels Jan 27, 2025

FNALbuild added code-checks-approved and removed code-checks-pending labels Jan 27, 2025

asanchezcastillo marked this pull request as ready for review January 27, 2025 16:26

FNALbuild added tests-started and removed tests-pending labels Feb 4, 2025

absolution1 reviewed Feb 5, 2025

View reviewed changes

Merge remote-tracking branch 'origin' into develop

a1f9638

FNALbuild added code-checks-pending tests-pending and removed requires-external code-checks-approved tests-started labels Feb 6, 2025

FNALbuild added code-checks-approved and removed code-checks-pending labels Feb 6, 2025

knoepfel reviewed Feb 11, 2025

View reviewed changes

asanchezcastillo mentioned this pull request Feb 12, 2025

Feature/acastill new pmt calibration SBNSoftware/sbndcode#633

Merged

6 tasks

Merge pull request #2 from LArSoft/develop

af6e19c

Update to larana v10_00_09

FNALbuild added code-checks-pending and removed code-checks-approved labels Feb 26, 2025

FNALbuild added code-checks-approved and removed code-checks-pending labels Feb 26, 2025

asanchezcastillo mentioned this pull request Feb 27, 2025

Feature/acastill opdetwaveform compatibility DUNE/duneopdet#83

Draft

knoepfel marked this pull request as draft May 19, 2025 15:17

knoepfel removed this from LArSoft pull requests Dec 15, 2025

		_peak_thresh_by_channel = pset.get<bool>("ThreshByChannel", false);
		_peak_thresh_vector = pset.get<std::vector<double>>("PeakThreshVector", {});

		_adc_thres_by_channel = pset.get<bool>("ADCThresholdByChannel", false);

		_adc_thres_vector = pset.get<std::vector<float>>("ADCThresVector", {});

Conversation

asanchezcastillo commented Jan 22, 2025

Uh oh!

FNALbuild commented Jan 22, 2025

Uh oh!

FNALbuild commented Jan 22, 2025

Uh oh!

FNALbuild commented Jan 22, 2025

Uh oh!

FNALbuild commented Jan 27, 2025

Uh oh!

FNALbuild commented Jan 27, 2025

Uh oh!

FNALbuild commented Jan 27, 2025

Uh oh!

knoepfel commented Jan 27, 2025

Uh oh!

asanchezcastillo commented Jan 27, 2025

Uh oh!

absolution1 commented Feb 4, 2025

Uh oh!

FNALbuild commented Feb 4, 2025

Uh oh!

FNALbuild commented Feb 4, 2025

Uh oh!

FNALbuild commented Feb 5, 2025

Uh oh!

absolution1 left a comment

Choose a reason for hiding this comment

Uh oh!

absolution1 Feb 5, 2025

Choose a reason for hiding this comment

Uh oh!

knoepfel Feb 11, 2025

Choose a reason for hiding this comment

Uh oh!

absolution1 Feb 5, 2025

Choose a reason for hiding this comment

Uh oh!

absolution1 Feb 5, 2025

Choose a reason for hiding this comment

Uh oh!

absolution1 Feb 5, 2025

Choose a reason for hiding this comment

Uh oh!

FNALbuild commented Feb 6, 2025

Uh oh!

FNALbuild commented Feb 6, 2025

Uh oh!

FNALbuild commented Feb 6, 2025

Uh oh!

knoepfel left a comment

Choose a reason for hiding this comment

Uh oh!

knoepfel Feb 11, 2025

Choose a reason for hiding this comment

Uh oh!

knoepfel commented Feb 24, 2025

Uh oh!

FNALbuild commented Feb 26, 2025

Uh oh!

FNALbuild commented Feb 26, 2025

Uh oh!

FNALbuild commented Feb 26, 2025

Uh oh!

asanchezcastillo commented Feb 27, 2025

Uh oh!

lgarren commented Mar 10, 2025

Uh oh!

knoepfel commented May 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants