Updates for first reprocessing of PDS products in spring productions by mvicenzi · Pull Request #876 · SBNSoftware/icaruscode

mvicenzi · 2025-12-08T22:14:22Z

This PR contains the required updates and fhicls to enable the first reprocessing of light products for the SBN2025 spring productions (both MC and data). In particular, the following changes are implemented:

MC2 tune (SPR + sim gain tune)
MC3/4 tune (SPR + sim gain tune)
Simulation of timing delays from database in overlays
Noise in MC waveform turned off for overlays
Overlay waveform chopping
DetSim + Overlay stage reprocessing fhicl(s)
Reco gain tune for run2/3/4
SPEArea database for optical reconstruction in data/overlays
OpHit recalibrator module for data
formally same reconstruction algorithms for data and MC (including pedestal's)
“MCStage0” reprocessing fhicl(s)
Data “stage0” reprocessing fhicl(s)

This PR depends on:

icarus_data update with new SPR responses + new SPRArea database
Feature/gp fast gaus opt icarusalg#97
Feature/gp plane crossers sbnalg#5

…plate It used to end when the template SPR goes under 1e-4 ADC#. Now it needs to stay within +/- 1w-4 ADC# for 20 nanoseconds. This should make it possible to include undershootings. All these parameters are currently hard-coded.

…icles that might cross cathode

Good catches on names and strings. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Trigger simulation modules now support input tags with process name [private]

Also requires the replacement of obsolete CreateAssns().

* removed unused headers and dependencies, added missing ones * updated exception error message * using `make_unique()` instead of `new`

Algorithmic code is still there, probably unused.

Also fixed a preexisting issue with (obsolete) FHiCL job configurations.

SPE Area DB

PetrilloAtWork

I could not find anything positively wrong.
However, I feel this is set up to add the n+1 layer of nightmare to configuration maintainers: with the fact that we discussed the workflow patterns many times together, and having laid out in front of me all the changes, I still had very hard time understanding the meaning of many of the choices, and their reasons. This may be mitigated by generously adding inline explanations or, if available, references to external documentation (like clear DocDB presentations). Especially useful would be to add a general introduction to many of the job configurations, to help people do fewer mistakes when they copy those into new ones for different data periods or samples.
I leave this unapproved (not disapproved either) for the first round. We (you and the release managers, mostly) may decide on the priorities of the changes suggested in this review with the merge of as-is material.

(Oh, there is actually one explicit request of changing the name of speAreas.fcl)

PetrilloAtWork · 2026-02-24T00:09:22Z

icaruscode/Decode/OverlayProducts_module.cc

+      // FIXME FIXME: temporary override to chop off flat tails after data waveform ends
+      // timing shift between data/mc event should be tuned so that this never (?) happens!
+	    //adcVec.push_back( overlayOpWave.baseline + (wvfm[idxWvfmEntry] - simBaseline) );


Since now this while is a pointless loop, I suggest to comment the whole loop out and replace it with idxWvfmEntry = wvfm.size(); (for good measure; currently idxWvfmEntry is not used any further).
In addition, to increase the visibility of the FIXME comment, I would add it in Doxygen format (a @todo block).

PetrilloAtWork · 2026-02-24T00:14:43Z

icaruscode/Generators/SelectCathodeCrossingGenParticles_module.cc

+ *    fraction of each dimension the cathode is extended for the purpose of
+ *    determining the crossing of the particle (see the documentation above).
+ * * `LogCategory` (string, default: `SelectCathodeCrossingGenParticles`):
+ *   message facility stream name where to write console messages to.


Suggested change

* message facility stream name where to write console messages to.

* message facility stream name where to write console messages to.

PetrilloAtWork · 2026-02-24T00:20:00Z

icaruscode/Generators/SelectCathodeCrossingGenParticles_module.cc

Ah, a most excellent module, if I may say so.

PetrilloAtWork · 2026-02-24T00:58:23Z

icaruscode/PMT/Algorithms/PMTsimulationAlg.cxx

+    double totalDelay = 0;
+    if(fParams.doTimingDelays) {
+      totalDelay -= fParams.timingDelays->getLaserCorrections(channel);
+      totalDelay -= fParams.timingDelays->getCosmicsCorrections(channel);
+    }
+    microsecond const timeDelay { totalDelay };


I suggest the delay to be represented immediately in microseconds, to explicitly and directly make a statement of each individual correction:

Suggested change

double totalDelay = 0;

if(fParams.doTimingDelays) {

totalDelay -= fParams.timingDelays->getLaserCorrections(channel);

totalDelay -= fParams.timingDelays->getCosmicsCorrections(channel);

}

microsecond const timeDelay { totalDelay };

microsecond totalDelay = 0_us;

if(fParams.doTimingDelays) {

totalDelay -= microseconds{ fParams.timingDelays->getLaserCorrections(channel) };

totalDelay -= microseconds{ fParams.timingDelays->getCosmicsCorrections(channel) };

}

However, note that the conversions of the delay components needs to be a time interval (util::quantities::intervals::microseconds) rather than a time point (as in microsecond); in the example it is assumed that it was somewhere stated that using microseconds = util::quantities::intervals::microseconds;.

PetrilloAtWork · 2026-02-24T01:05:32Z

icaruscode/PMT/Algorithms/PMTsimulationAlg.h

    std::uint64_t beamGateTimestamp,
    detinfo::LArProperties const& larProp,
    detinfo::DetectorClocksData const& detClocks,
+    icarusDB::PMTTimingCorrections const& timingDelays,


Having a reference here forces the service provider to exist.
I suggest replacing the interface to use a nullable pointer, so that when it is not needed the caller can opt not to load the service at all.
This should be paired to an assertion (along the line of assert(!doApplyTimingDelays || timingDelay);) in the algorithm to make sure that when needed the service provider is available.

It does break the symmetry with the other service providers in the interface, but that may be actually good as long as they are mandatory and this is not always so.

PetrilloAtWork · 2026-02-26T00:00:36Z

fcl/detsim/partial/detsim_opdetonly_icarus_overlay_reprocessing_run2.fcl

Again some comment words about what this configuration does and what needs reprocessing would help.

PetrilloAtWork · 2026-02-26T00:46:02Z

fcl/reco/Definitions/stage0_icarus_driver_common.fcl

  IICARUSChannelMap:           @local::icarus_channelmappinggservice
  IPMTTimingCorrectionService: @local::icarus_pmttimingservice
                               @table::icarus_wirecalibration_minimum_services
+  IPhotonCalibratorService: @local::icarus_photon_calibration


The @local is misaligned, and I think the idea was to have the single services included before the @table. All of this does not really matter, but if you get a chance fix it.

PetrilloAtWork · 2026-02-26T01:18:16Z

fcl/reco/Definitions/stage0_icarus_mc_defs.fcl

+    ophituncorrected:  @local::icarus_ophit_data
+    ophit:             @local::icarus_ophit_MC


This confuses me a lot.
Maybe specify which correction is left out?

Suggested change

ophituncorrected: @local::icarus_ophit_data

ophit: @local::icarus_ophit_MC

# ophit will produce recob::OpHit with all needed time corrections:

# - in pure MC, that is actually no correction at all (configuration below);

# - in data and overlay, time correction is needed so ophit must be

# overridden later to become the time correction module

# (run on top of uncorrected hits)

# ophituncorrected never includes time corrections

ophituncorrected: @local::icarus_ophit_data # from `icarus_ophitfinder.fcl`

ophit: @local::icarus_ophit_MC # from `icarus_ophitfinder.fcl`

PetrilloAtWork · 2026-02-26T01:18:53Z

fcl/reco/Definitions/stage0_icarus_mc_defs.fcl

    crthit:            @local::standard_crtsimhitproducer
    crttrack:          @local::standard_crttrackproducer
-    crtpmt: 	       @local::standard_crtpmtmatchingproducer
+    crtpmt: 	         @local::standard_crtpmtmatchingproducer


I disapprove this change.

Suggested change

crtpmt: @local::standard_crtpmtmatchingproducer

crtpmt: @local::standard_crtpmtmatchingproducer

PetrilloAtWork · 2026-02-26T01:30:39Z

icaruscode/PMT/opdetsim_pmt_icarus.fcl

+  @table::icarus_pmtsimulationalg_standard_run2
 }

+icarus_simpmt_run4: 


Add a comment on the fact that icarus_simpmt_run4 holds for Run3 too.
Or else add its own configuration:

icarus_simpmt_run3: @local::icarus_simpmt_run4

mvicenzi and others added 30 commits May 23, 2025 00:17

fix sim label for PDFastSim

3cab2e9

detsim_opdetonly_icarus.fcl: simulate only optical detector digitization

b6c5c1a

Updated FastGaussianNoiseGeneratorAlg documentation

b129249

SelectCathodeCrossingGenParticles: selects generated events with part…

3f467fb

…icles that might cross cathode

Merge branch 'mvicenzi/quick_fix' into feature/mvicenzi_repro_step1

0dc5ec4

make MC2-var4 tune standard

0d3e9ff

simulate pmt timing delays in mc

1428556

disable pmt mc noise for overlays

9d43105

add ophituncorrected + ophit flow for mc

d655888

update stage0 flow for overlays

6f0342b

small fix

7e655c6

prelimanary reco gain tuning

8726590

add SPRRun3, make SPRRun2 default

0fe2b8e

nullify flash calibration

a138d80

add run3 opdet tune, make run2 default

e9a9bea

change run3->run4, update gain

e918fe8

Merge branch 'release/SBN2025A' into feature/mvicenzi_repro_step1

677563f

add run4 detsim fhicls

99f4413

first attempt at detsim reprocessing fhicl

b90cfb5

fix typo

468cb3f

Tell CMake of sbnalg dependency

9667657

Trigger simulation modules: support input tag with process name

8c1e40b

Apply suggestions from GIT Copilot code review

937b4b6

Good catches on names and strings. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Merge pull request #870 from PetrilloAtWork/feature/gp_LVDSgatesFix

ab9ecf2

Trigger simulation modules now support input tags with process name [private]

fix to detsim reprocessing

a80e88b

reprocessing opdetonly overlay fhicl

fe5e072

leave default tags

2142449

add initial recalibrator module

0c07aeb

fix compilation for recalibrator

16be497

Michael Carrigan and others added 21 commits December 12, 2025 10:36

basic struct/map for holding spe areas

eb6bdb1

working version getting area from database and initial database file

0bd1735

updated tags in db file

6987683

add fcl file for debugging spe area service

667d576

ICARUSFlashFinder: added Doxygen-style header

d7b1c3a

ICARUSFlashFinder: turned into shared module

d91e636

Also requires the replacement of obsolete CreateAssns().

ICARUSFlashFinder: FHiCL configuration validation

4a92ddf

ICARUSFlashFinder: code updates

e433639

* removed unused headers and dependencies, added missing ones * updated exception error message * using `make_unique()` instead of `new`

ICARUSFlashFinder: removed hit recalibration (PECalib)

93fcc2c

Algorithmic code is still there, probably unused.

dump_opflashes_icarus.fcl: dump reconstructed flashes on text file

4d8d9f4

Adopting corrections from GitHub Copilot

15541a4

Removed obsolete icarus_flashcalib.fcl

a019654

Also fixed a preexisting issue with (obsolete) FHiCL job configurations.

PMT calibration service code review

6c188f2

update pmt calibration service provider

4db5efb

Merge branch 'release/SBN2025A' into feature/mvicenzi_repro_step1

ba917d5

Merge pull request #880 from carriganm95/carriganm95/speAreaTool

74b471e

SPE Area DB

removed db file + copilot suggestions

a4b8813

interface SPEArea DB with ophit recalibrator

7efe1eb

fix cout of current areatag

5ad6287

rework stage0 optical workflow for data, mc, overlay

c2df7b8

simulate pmt timing delays only for overlay flow

d7c5c93

mvicenzi removed the request for review from gputnam January 19, 2026 23:05

remove ophituncurrected from legacy mc flow

2a7f75e

mvicenzi force-pushed the feature/mvicenzi_repro_step1 branch from fdd76d8 to 419088b Compare January 19, 2026 23:45

tune mcophit

c435bf2

mvicenzi force-pushed the feature/mvicenzi_repro_step1 branch from 419088b to c435bf2 Compare January 20, 2026 00:01

make applytimingdelays explicit

d5024e9

mvicenzi marked this pull request as ready for review January 20, 2026 00:16

add run3/4 stage0 overlay light tunes

3139d6f

PetrilloAtWork reviewed Feb 26, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates for first reprocessing of PDS products in spring productions#876

Updates for first reprocessing of PDS products in spring productions#876
mvicenzi wants to merge 69 commits intorelease/SBN2025Afrom
feature/mvicenzi_repro_step1

mvicenzi commented Dec 8, 2025 •

edited by PetrilloAtWork

Loading

Uh oh!

PetrilloAtWork left a comment

Uh oh!

PetrilloAtWork Feb 24, 2026

Uh oh!

PetrilloAtWork Feb 24, 2026

Uh oh!

PetrilloAtWork Feb 24, 2026

Uh oh!

PetrilloAtWork Feb 24, 2026

Uh oh!

PetrilloAtWork Feb 24, 2026

Uh oh!

PetrilloAtWork Feb 26, 2026

Uh oh!

PetrilloAtWork Feb 26, 2026

Uh oh!

PetrilloAtWork Feb 26, 2026

Uh oh!

PetrilloAtWork Feb 26, 2026

Uh oh!

PetrilloAtWork Feb 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	* message facility stream name where to write console messages to.
	* message facility stream name where to write console messages to.

		ophituncorrected: @local::icarus_ophit_data
		ophit: @local::icarus_ophit_MC

-    ophituncorrected:  @local::icarus_ophit_data
-    ophit:             @local::icarus_ophit_MC
+    # ophit will produce recob::OpHit with all needed time corrections:
+    # - in pure MC, that is actually no correction at all (configuration below);
+    # - in data and overlay, time correction is needed so ophit must be
+    #     overridden later to become the time correction module
+    #     (run on top of uncorrected hits)
+    # ophituncorrected never includes time corrections
+    ophituncorrected:  @local::icarus_ophit_data  # from `icarus_ophitfinder.fcl`
+    ophit:             @local::icarus_ophit_MC    # from `icarus_ophitfinder.fcl`

	crtpmt: @local::standard_crtpmtmatchingproducer
	crtpmt: @local::standard_crtpmtmatchingproducer

Conversation

mvicenzi commented Dec 8, 2025 • edited by PetrilloAtWork Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PetrilloAtWork left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mvicenzi commented Dec 8, 2025 •

edited by PetrilloAtWork

Loading