Feature/ascarpel timing pmt crt by ascarpel · Pull Request #440 · SBNSoftware/icaruscode

ascarpel · 2022-09-07T15:43:09Z

This PR introduces several changes related to the PMT system and the way timing is corrected and calculated:

In Decode/ChannelMapping: modified the PMTChannelMapping to include the laser optical channel in preparation of a future PR including the code for the laser calibration. It should be harmless
In PMT/ICARUSFlashAssAna_module.cc new variable are added to the TTree holding the OpFlash information. The new variables group together OpHit-level information related to the same PMT, in order to have a more intuitive overview of the components of each single OpFlash . It should be harmless although it would be good to verify the final data size of the calibration ntuples after this modification and verify if compatible with the space allocated
Files Timing/PMTTimingCorrectionService_service.cc , Timing/PMTTimingCorrectionsProvider.* are added to include a new LArSoft service that interface with the PostgreSQL calibration database and load the database entries given a run number. It should be verified the robustness of the code proposed and if there is room to improve its speed and performance.
A new data product Timing/DataProducts/PMTWaveformTimeCorrection.* with the corresponding extractor PMTWaveformTimeCorrectionExtractor.* is designed to calculate and hold the time correction coming from the sampled reference signal on the spare channel of the CAEN1730V digitizers. This would allow dropping the corresponding waveform (the ones that now are saved with the instances “trigprm”, “BNB”, “NuMI”). The data product is saved following the same convention followed for the waveforms
A module called Timing/OpHitTimingCorrection_module.cc which takes the OpHit data products vector and generates a new OpHit data product list with PeakTimeAbs() and PeakTime()corrected for the delays calculated using the laser and muon calibration. The choice to apply that correction to the OpHit was made to preserve the representation of the OpWaveform::TimeStamp()
Changes to DaqDecoderICARUSPMT_module.cc to implement the correction of the first two bullet point above and produces the PMTWaveformTimeCorrection data-products. It also recalculates the OpWaveform::TimeStamp() recalculating the offset starting from the sample of the "trgprim" labelled waveform. It should be verified if this approach is valid and consistent with the remaining part of the decoder logic.

There should be backward compatibility with data collected previously, however, since most of the changes apply to the stage0 step of the production. Older samples must be reprocessed to include the changes here introduced

…vices

mmrosenberg · 2022-09-07T19:59:08Z

trigger build

FNALbuild · 2022-09-07T20:02:24Z

✔️ CI build for LArSoft Succeeded on slf7 for c7:prof -- details available through the CI dashboard

FNALbuild · 2022-09-07T20:02:25Z

✔️ CI build for LArSoft Succeeded on slf7 for e20:prof -- details available through the CI dashboard

FNALbuild · 2022-09-07T20:36:49Z

⚠️ CI build for ICARUS Warning at phase ci_tests ICARUS on slf7 for c7:prof - ignored warnings for build -- details available through the CI dashboard

🚨 For more details about the warning phase, check the ci_tests ICARUS phase logs

parent CI build details are available through the CI dashboard

FNALbuild · 2022-09-07T20:38:17Z

⚠️ CI build for ICARUS Warning at phase ci_tests ICARUS on slf7 for e20:prof - ignored warnings for build -- details available through the CI dashboard

🚨 For more details about the warning phase, check the ci_tests ICARUS phase logs

parent CI build details are available through the CI dashboard

* the update with a new run was already removed from the interface * map access replaced with read-only one * default values for missing entries explicitly implemented

* algorithms use `icarusDB::PMTTimingCorrections` interface (can store it in a constant reference data member) * art modules provide them the object with that interface via the `icarusDB::IPMTTimingCorrectionService` service interface: `lar::providerFrom<icarusDB::IPMTTimingCorrectionService>()` or equivalent returns the pointer the algorithms need * configuration must include the service `IPMTTimingCorrectionService` and specify its implementation (currently only `PMTTimingCorrectionService` is provided

Previously, the corrections were in ophitcorr, which was then used for creating flashes.

PetrilloAtWork · 2022-09-25T05:08:16Z

The last commit is a fix triggered by a report by @francescopoppi and @aheggest who did not see the time correction on the hits in some output trees. The branch was creating the ophitcorr data product from ophit, and using the former for reconstructing the flashes, which is all well and good as long as you use the flashes. But some trees (simpleLightAna and probably any CAF tree) used to pick ophit which still did not have the correction.
Now ophit is corrected, and the uncorrected... ophituncorrected is just an accident of the workflow. We can even drop it if it bothers (edit: in fact, that's what the Stage0 configuration does). The corrections are saved in their own data product, in case one wanted to attempt to change or undo them.

PetrilloAtWork

The main requests were addressed. I have become part of the solution, so my review is not fair any more.
Thus, I hereby approve.

PetrilloAtWork · 2022-09-07T20:23:25Z

+            // Get the laserChannelNumber 
+            char laserChannelBuffer[10];
+            getStringValue(tuple, 7, laserChannelBuffer, sizeof(laserChannelBuffer), &error);
+            if (error) throw std::runtime_error("Encountered error when trying to recover the PMT laser channel label");


It would be nice to know which error that was. e.g.:

Suggested change

if (error) throw std::runtime_error("Encountered error when trying to recover the PMT laser channel label");

if (error) throw std::runtime_error("Encountered error (code: " + std::to_string(error) + ") when trying to recover the PMT laser channel label");

This of course is a general comment for the class (and of course I don't expect you to change this all the way in this context).

PetrilloAtWork · 2022-09-07T20:33:33Z

+            getStringValue(tuple, 7, laserChannelBuffer, sizeof(laserChannelBuffer), &error);
+            if (error) throw std::runtime_error("Encountered error when trying to recover the PMT laser channel label");
+            std::string laserChannelLabel(laserChannelBuffer, 2, sizeof(laserChannelBuffer)); //sizeof(digitizerBuffer));
+            unsigned int laserChannel = std::stol(laserChannelLabel);  // try-catch syntax for stol or not necessary ? 


Necessary if we want meaningful error messages (remember the error from the trigger decoder? the entirety of the message was: stoi).

Suggested change

unsigned int laserChannel = std::stol(laserChannelLabel); // try-catch syntax for stol or not necessary ?

unsigned int laserChannel;

try {

laserChannel = std::stol(laserChannelLabel);

}

catch(std::logic_error const& e) {

throw std::runtime_error("Invalid laser channel number from digitizer " + digitizerLabel + ": " + laserChannelLabel + " (\"" + e.what() + "\")";

}

PetrilloAtWork · 2022-09-07T20:41:04Z


+    // Read the laser channel 
+    std::string laserChannelLabel  = argv[7]; // format is L-<number> . <number> is int from [1-41]
+    unsigned int laserChannel      = std::stol(laserChannelLabel.substr(2,4));  // try-catch syntax for stol or not necessary ? 


Necessary if we want meaningful error messages (remember my last comment? yes, this is C&P).

Suggested change

unsigned int laserChannel = std::stol(laserChannelLabel.substr(2,4)); // try-catch syntax for stol or not necessary ?

unsigned int laserChannel;

try {

laserChannel = std::stol(laserChannelLabel.substr(2,4));

}

catch(std::logic_error const& e) {

throw std::runtime_error("Invalid laser channel number for fragment " + std::to_string(fragmentID) + " channel " std::to_string(digitizerChannelNo) + ": " + laserChannelLabel + " (\"" + e.what() + "\")";

}

PetrilloAtWork · 2022-09-07T21:04:08Z

     *         Then define the function interface to fill these data structures 
     */
-    using DigitizerChannelChannelIDPair    = std::pair<size_t,size_t>;
+    using DigitizerChannelChannelIDPair    = std::tuple<size_t,size_t,size_t>; // std::tuple<DigitizerChannel, ChannelID, LaserChannel>


Lui, lei te... un paio in tre. 😉

PetrilloAtWork · 2022-09-07T21:06:20Z

  bool const fRequireBoardConfig;
+
+  /// String of the instance to use for the time corrections
+  std::string fCorrectionInstance;


To follow the pattern in the module:

Suggested change

std::string fCorrectionInstance;

std::string const fCorrectionInstance;

(not a bit deal, but it prevents "accidental" modifications)

PetrilloAtWork · 2022-09-07T21:23:59Z

+  for (std::string const& instanceName: getAllInstanceNames()){
+    if( instanceName.empty() ) continue;
+    produces<std::vector<icarus::timing::PMTWaveformTimeCorrection>>(instanceName);


I think there is a merge issue here.

Suggested change

for (std::string const& instanceName: getAllInstanceNames()){

if( instanceName.empty() ) continue;

produces<std::vector<icarus::timing::PMTWaveformTimeCorrection>>(instanceName);

if (!fSkipWaveforms) {

for (std::string const& instanceName: getAllInstanceNames())

produces<std::vector<raw::OpDetWaveform>>(instanceName);

}

for (std::string const& instanceName: getAllInstanceNames()){

if( instanceName.empty() ) continue;

produces<std::vector<icarus::timing::PMTWaveformTimeCorrection>>(instanceName);

My suggestion here honours fSkipWaveforms in writing the waveforms only if requested, but it always writes the correction. In alternative, the corrections may be written only when the waveforms are.
I don't believe there is one rightful solution here, since the SkipWaveforms option is there just for debug.

PetrilloAtWork · 2022-09-07T21:24:16Z

+
+  produces<std::vector<raw::OpDetWaveform>>();


Tail of the merge issue above.

Suggested change

produces<std::vector<raw::OpDetWaveform>>();

PetrilloAtWork · 2022-09-07T21:57:02Z

    = [&digitizerChannelVec](unsigned short int channelNumber) -> raw::Channel_t
    {
-      for (auto const [ chNo, chID ]: digitizerChannelVec)
+      for (auto const [ chNo, chID, _ ]: digitizerChannelVec) // too pythonic? 


I have seen that (rarely) in C++ too. Maybe from people used to Python...

SFBayLaser

I am not entirely sure why I was selected as a reviewer here since I am not familiar with the details of this code. I see that Gianluca has done a comprehensive review and given his long track record of careful work if he has approved then it is fine for me.

rennney · 2022-09-26T17:40:28Z

trigger build

FNALbuild · 2022-09-26T18:15:39Z

✔️ CI build for LArSoft Succeeded on slf7 for c7:prof -- details available through the CI dashboard

FNALbuild · 2022-09-26T18:41:48Z

✔️ CI build for LArSoft Succeeded on slf7 for e20:prof -- details available through the CI dashboard

FNALbuild · 2022-09-26T19:02:49Z

❌ CI build for ICARUS Failed at phase ci_tests ICARUS on slf7 for c7:prof - ignored warnings for build -- details available through the CI dashboard

🚨 For more details about the failed phase, check the ci_tests ICARUS phase logs

parent CI build details are available through the CI dashboard

FNALbuild · 2022-09-26T19:40:35Z

❌ CI build for ICARUS Failed at phase ci_tests ICARUS on slf7 for e20:prof - ignored warnings for build -- details available through the CI dashboard

🚨 For more details about the failed phase, check the ci_tests ICARUS phase logs

parent CI build details are available through the CI dashboard

PetrilloAtWork · 2022-09-26T20:40:56Z

I am not entirely sure why I was selected as a reviewer here since I am not familiar with the details of this code. I see that Gianluca has done a comprehensive review and given his long track record of careful work if he has approved then it is fine for me.

AKA: "if the branch messes up anything, it's his fault."

ascarpel · 2022-10-05T16:29:29Z

After a quick investigation, the CI test failure is related to the change of the Optical Hit reconstruction process name. the former ophit is now ophituncorrected while since this PR the ophit relates to the OpHit corrected including timing. The new names should be appropriately homogenized across the framework, in particular for Monte Carlo configuration files.

ascarpel · 2022-10-05T16:31:06Z

commit 60121d3 should fix the above

rennney · 2022-10-05T16:35:17Z

trigger build

FNALbuild · 2022-10-05T16:38:47Z

✔️ CI build for LArSoft Succeeded on slf7 for c7:prof -- details available through the CI dashboard

FNALbuild · 2022-10-05T16:38:48Z

✔️ CI build for LArSoft Succeeded on slf7 for e20:prof -- details available through the CI dashboard

FNALbuild · 2022-10-05T17:15:09Z

❌ CI build for ICARUS Failed at phase ci_tests ICARUS on slf7 for c7:prof - ignored warnings for build -- details available through the CI dashboard

🚨 For more details about the failed phase, check the ci_tests ICARUS phase logs

parent CI build details are available through the CI dashboard

FNALbuild · 2022-10-05T17:29:55Z

❌ CI build for ICARUS Failed at phase ci_tests ICARUS on slf7 for e20:prof - ignored warnings for build -- details available through the CI dashboard

🚨 For more details about the failed phase, check the ci_tests ICARUS phase logs

parent CI build details are available through the CI dashboard

mmrosenberg · 2022-10-05T21:14:54Z

trigger build

FNALbuild · 2022-10-05T21:18:14Z

✔️ CI build for LArSoft Succeeded on slf7 for e20:prof -- details available through the CI dashboard

FNALbuild · 2022-10-05T21:18:23Z

✔️ CI build for LArSoft Succeeded on slf7 for c7:prof -- details available through the CI dashboard

FNALbuild · 2022-10-05T21:54:16Z

⚠️ CI build for ICARUS Warning at phase ci_tests ICARUS on slf7 for c7:prof - ignored warnings for build -- details available through the CI dashboard

🚨 For more details about the warning phase, check the ci_tests ICARUS phase logs

parent CI build details are available through the CI dashboard

FNALbuild · 2022-10-05T21:54:58Z

⚠️ CI build for ICARUS Warning at phase ci_tests ICARUS on slf7 for e20:prof - ignored warnings for build -- details available through the CI dashboard

🚨 For more details about the warning phase, check the ci_tests ICARUS phase logs

parent CI build details are available through the CI dashboard

rennney · 2022-10-12T11:25:55Z

@PetrilloAtWork Hi, we got a merge conflict with recent restructuring of icarus config files. Please have a look

… (feature/ascarpel_timing_pmt_crt, see Pull Request #440) with updates to fhicl files to include the CRT hits in stage 0. This became a bit complicated by the base of Andrea's feature branch being before the restructuring of fhicl files so I've had to reconcile that. As well, I moved the new PMT data product to the IcarusObj folder so we can be sure of where are data objects are. Note that I did not reconcile the fhicl files directly related to production running so some of the special case PMT fhicl files still need reconciliation.

…d_crt_stage0 Reconcile PR #440 with new fhicl file format, include CRT in stage 0

ascarpel added 20 commits May 13, 2022 09:32

Read from the channel mapping also the laser label

60121d3

Midway point commit

0156ac4

kickoff commit

eff349a

Commit that compiles. TODO:add trigger time and clock period from ser…

a0b69e3

…vices

Fixed clock services problems

8baa277

Fixed minor problems for better performances and reading of the code

a62a1ff

Some fixes and tests for the TimingCorrectionExtraction_module.cc

811417f

Merge branch 'develop' into feature/ascarpel_laserchannelmapping

74c786f

Smarter Flash Analyzer

ee509c7

add some CRT changes from F. Poppi

9748444

Merge branch 'develop' into feature/ascarpel_timing_pmt_crt

78efe91

Timing corrections

f3350a6

Moved the timing corrections to a service

8908d46

Add implementation of the function extracting the timing corrections

0d6cc45

Apply time corrections

bf6f485

Decode/BeamBits.h

ef5d4c1

Fixed small bug causing segmentation falut

483c43d

Add timing correction also for OpHit. General bug and stability tests

a3bceba

Removed unwanted files

87a3cbd

Merge conflicts fixed

70be31c

ascarpel requested review from PetrilloAtWork and SFBayLaser September 7, 2022 15:43

PetrilloAtWork added 3 commits September 7, 2022 21:43

PMTTimingCorrectionService now automatically updates on a new run.

f19b9e7

PMTTimingCorrectionService interface made constant

d15ec82

* the update with a new run was already removed from the interface * map access replaced with read-only one * default values for missing entries explicitly implemented

The default ophit data product is now time-corrected.

f02f971

Previously, the corrections were in ophitcorr, which was then used for creating flashes.

PetrilloAtWork approved these changes Sep 25, 2022

View reviewed changes

SFBayLaser approved these changes Sep 26, 2022

View reviewed changes

rennney mentioned this pull request Sep 30, 2022

Feature/aheggest crt t1 #459

Closed

solve conflicts with the label process name

cb8ca6d

PetrilloAtWork mentioned this pull request Oct 6, 2022

PMT timing corrections should not be applied on simulation #464

Open

SFBayLaser mentioned this pull request Oct 15, 2022

Reconcile PR #440 with new fhicl file format, include CRT in stage 0 #467

Merged

mmrosenberg added a commit that referenced this pull request Nov 1, 2022

Merge pull request #467 from SBNSoftware/feature/usher_merge_pr440_an…

00729d9

…d_crt_stage0 Reconcile PR #440 with new fhicl file format, include CRT in stage 0

mmrosenberg merged commit 5987b73 into develop Nov 1, 2022

	if (error) throw std::runtime_error("Encountered error when trying to recover the PMT laser channel label");
	if (error) throw std::runtime_error("Encountered error (code: " + std::to_string(error) + ") when trying to recover the PMT laser channel label");

-            unsigned int laserChannel = std::stol(laserChannelLabel);  // try-catch syntax for stol or not necessary ?
+            unsigned int laserChannel;
+            try {
+                laserChannel = std::stol(laserChannelLabel);
+            }
+            catch(std::logic_error const& e) {
+                throw std::runtime_error("Invalid laser channel number from digitizer " + digitizerLabel + ": " + laserChannelLabel + " (\"" + e.what() + "\")";
+            }

	std::string fCorrectionInstance;
	std::string const fCorrectionInstance;

Conversation

ascarpel commented Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmrosenberg commented Sep 7, 2022

Uh oh!

FNALbuild commented Sep 7, 2022

Uh oh!

FNALbuild commented Sep 7, 2022

Uh oh!

FNALbuild commented Sep 7, 2022

Uh oh!

FNALbuild commented Sep 7, 2022

Uh oh!

PetrilloAtWork commented Sep 25, 2022 • edited 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!

SFBayLaser left a comment

Choose a reason for hiding this comment

Uh oh!

rennney commented Sep 26, 2022

Uh oh!

FNALbuild commented Sep 26, 2022

Uh oh!

FNALbuild commented Sep 26, 2022

Uh oh!

FNALbuild commented Sep 26, 2022

Uh oh!

FNALbuild commented Sep 26, 2022

Uh oh!

PetrilloAtWork commented Sep 26, 2022

Uh oh!

ascarpel commented Oct 5, 2022

Uh oh!

ascarpel commented Oct 5, 2022

Uh oh!

rennney commented Oct 5, 2022

Uh oh!

FNALbuild commented Oct 5, 2022

Uh oh!

FNALbuild commented Oct 5, 2022

Uh oh!

FNALbuild commented Oct 5, 2022

Uh oh!

FNALbuild commented Oct 5, 2022

Uh oh!

mmrosenberg commented Oct 5, 2022

Uh oh!

FNALbuild commented Oct 5, 2022

Uh oh!

FNALbuild commented Oct 5, 2022

Uh oh!

FNALbuild commented Oct 5, 2022

Uh oh!

FNALbuild commented Oct 5, 2022

Uh oh!

rennney commented Oct 12, 2022

Uh oh!

Reviewers

Assignees

Labels

ascarpel commented Sep 7, 2022 •

edited

Loading

PetrilloAtWork commented Sep 25, 2022 •

edited

Loading