Gen2 Production PR: Manual Fix for GENIE timing bug in CAFMaker + Extend existing-data shift in CAFMaker by VCLanNguyen · Pull Request #675 · SBNSoftware/sbncode

VCLanNguyen · 2026-09-25T17:38:14Z

Description:

Adds SRTrueInteraction.prod_time/.dk2gen field
Adds SRMeVPrtl.prod_time
Assigns rec.mc.nu.generator in CAFMaker_module.cc — previously unset
Adds CorrectMCTiming() (MC only, nu.generator == kGENIE): computes a per-true-neutrino offset_ns = nu.prod_time + nu.dk2gen × 100/c, applies each slice's matched neutrino's offset to slc.truth, opt0/opt0_sec, barycenterFM, all fmatch*, crumbs_result, correctedOpFlash.*, matched CRT tracks
Extends pre-existing data-side shift (SBNDShiftCRTReference/SBNDShiftPMTReference) to also cover opt0_sec, barycenterFM, fmatch*, crumbs_result

Validation:

See slide 1-26 on docdb#49588

Dependency:

Gen2 Production:

sbncode: #675
sbnanaobj: SBNSoftware/sbnanaobj#199
sbndcode: SBNSoftware/sbndcode#967

Develop:

sbncode: #676 -- to mirror description bullet 1/2/3
sbnanaobj: SBNSoftware/sbnanaobj#200 -- to mirror description bullet 1/2/3

Checklist:

Have you added a label? (bug/enhancement/physics etc.)
Have you assigned at least 1 reviewer?
Is this PR related to an open issue / project?
Does this PR affect CAF data format? If so, please assign a CAF maintainer as additional reviewer. @Gianluca Petrillo and @thomas Jones
Does this PR require merging another PR in a different repository (such as sbnanobj/sbnobj etc.)? If so, please link it in the description.
Are you submitting this PR on behalf of someone else who made the code changes? If so, please mention them in the description.

…ycenterFM, fmatch variants, and CRUMBS features Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lxqnx6r8HqPPywzXcxBahz

…Maker Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lxqnx6r8HqPPywzXcxBahz

…coped reco objects Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lxqnx6r8HqPPywzXcxBahz

Fields shifted by CorrectMCTiming()/SBNDShiftCRTReference()/SBNDShiftPMTReference()/ FixPMTReferenceTimes() came from optional slice-scoped associations (OpT0Finder candidate, TPCPMTBarycenterMatch, SimpleFlashMatch, CRUMBSResult) that aren't guaranteed to exist for a given slice. Shifting an absent association's field turned its honest "not filled" sentinel into a bogus near-default value that looked filled but wasn't. Each field is now guarded against its own struct's actual unfilled convention (NaN for structs with no setDefault(), -9999. for setDefault()-based structs and the upstream SimpleFlashMatch producer's own sentinel), confirmed individually from source and empirically rather than assumed uniform.

CorrectMCTiming() recovers a dk2nu-specific bug (missing meson decay time in the stored vertex time) and so should only ever touch interactions that came from GENIE. But rec.mc.nu is filled from whatever generator produced MCTruth under the configured producer label, not GENIE specifically -- HNL/dissonant- Higgs productions (MeVPrtlGen) reuse the same "generator" label and push their own MCTruth there too. The correction happened to still skip those in current production configs, but only because MeVPrtlGen never produces the bsim::Dk2Nu product the correction depends on -- incidental, not an explicit guarantee. Sets SRTrueInteraction.generator (declared, previously never assigned) from whether a simb::GTruth was associated with the interaction -- GTruth only ever comes from GENIE -- and gates the correction on it directly.

SBNDShiftPMTReference() was shifting slc.correctedOpFlash.OpFlashT0/ .OpFlashT0Corrected by the SBND frame shift, but for data these fields are already reference-frame corrected upstream: LightPropagationCorrection_module.cc computes them from the real SPECTDC RWM/trigger timestamps in its data-only (!fIsMC) branch. Shifting them again here double-corrected the reference time. Same category as soft_trig.flash_peaktime/sbnd_crt_veto.sp_time, already left alone for this reason. MC is unaffected: for MC that module's !fIsMC branch is skipped, so correctedOpFlash is unshifted at the producer level and CorrectMCTiming()'s own dk2nu-offset correction of it remains necessary.

…default simb::MCFlux::fdk2gen defaults to -999. (its own Reset()), a different sentinel than the -9999. convention every other guard in this function checks. Without this, an interaction with an unfilled dk2gen would compute a wrong-but-not-NaN offset_ns and silently corrupt every field CorrectMCTiming shifts, the same failure class as the earlier -9999. guard fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…tions Trim CorrectMCTiming()/SBNDShiftCRTReference()/SBNDShiftPMTReference()/ FixPMTReferenceTimes() comments from prose paragraphs to short per-field labels, and record that SRSBNDCRTVeto.sp_time, SRSoftwareTrigger.flash_peaktime, and correctedOpFlash.OpFlashT0/.OpFlashT0Corrected (data path) are already reference-frame corrected upstream in their own producer modules (CRTVeto, SoftwareTrigger, LightPropagationCorrection) rather than left as an open watch-list item. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…nerator Previously any MCTruth without an associated GTruth (i.e. non-GENIE) was tagged kUnknownGenerator, even though MeVPrtlGen_module.cc pushes a matching MeVPrtlTruth into the same event at the same index as its MCTruth (confirmed in its produce(): the two collections are always pushed back-to-back in the same loop iteration). Use that index alignment -- the same assumption this function already relies on for mcfluxes.at(i)/dk2nus.at(i) -- to tag those entries kMeVPrtl instead. caf::kMeVPrtl already existed in SREnums.h but nothing was setting it. CorrectMCTiming()'s existing `generator != caf::kGENIE` guard needs no change: kMeVPrtl still isn't kGENIE, so those slices are still skipped. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

This function runs unconditionally for both MC and data with no detector gate (called for ICARUS too), so guarding it is out of scope for this SBND-only dk2nu/gate-shift project -- back to unconditionally shifting fmatch.time/barycenterFM.flashTime/.flashFirstHit, matching pre-guard-pass behavior byte-for-byte (comment included), so ICARUS CAF output is unaffected.

Companion to the sbnanaobj SRMeVPrtl::prod_time field: sources it from MeVPrtlTruth::meson_dpos_beamcoord.T(), the clean pre-beam-offset meson decay time (already threaded from the BNB flux ntuple's ini_t[0]). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

VCLanNguyen · 2026-10-07T20:30:32Z

Hi @tjones79 and/or @PetrilloAtWork can you guys have a look to review? I have attached the docdb that has validation plots to cover all the expected changes (and also unchanges).

tjones79

Just the one comment. Everything else looks good to me. Thanks :)

tjones79 · 2026-10-12T10:08:14Z

+    offset_ns[i] = nu.prod_time + nu.dk2gen * 100. / kSpeedOfLight;
+    double const delta_us = offset_ns[i] / 1000.;
+
+    nu.time += delta_us;


Looking at your validation plots (Thank you very much for these; they are extremely helpful!!)...

Your plots for rec.mc.nu.time and the prim truth times (Slides 7 and 8) show a clear ~350 ns shift. But the current code in the PR adds delta_us to these fields, which, naively, I would expect only to shift them by ~0.35 ns. Should this not actually be += offset_ns[i]?

(Looking at your other PR: https://github.com/SBNSoftware/sbnanaobj/pull/200/changes#diff-14e572d921caad14ab8a3f1b14c6af56c1dec7e9d457dce906c7024ed0e89d1bL57 The interaction time is also explicitly defined in ns in your comment)

PetrilloAtWork

I feel I am missing an important point of this PR.
It unconditionally shifts all (or many) times to the time reference of the decay of the meson which originated the neutrino/particle. Is that correct? if so, why?
I see the mention of a bug in dk2nu, but it's not clear to me how this correction would leave alone input data not affected by that bug.
I have left some technical comments across the code. Once those are resolved, for me this PR is good to go in the target branch. But until the questions above are answered, I think the PR in this state should not be merged into develop.

PetrilloAtWork · 2026-10-12T20:45:44Z

  void SBNDShiftCRTReference(StandardRecord &rec, double SBNDFrame) const;
  void SBNDShiftPMTReference(StandardRecord &rec, double SBNDFrame) const;

+  void CorrectMCTiming(StandardRecord &rec) const;


Please add Doxygen documentation here about the corrections that are included in this function.

PetrilloAtWork · 2026-10-12T20:53:15Z

+
+void CAFMaker::CorrectMCTiming(StandardRecord &rec) const {
+
+  std::cout << "CAFMaker::CorrectMCTiming: running" << std::endl;


I recommend to omit this line — it's not so informative in the log.
In alternative, it can be sent to the message facility at DEBUG level:

Suggested change

std::cout << "CAFMaker::CorrectMCTiming: running" << std::endl;

mf::LogDebug("CAFMaker") << "CorrectMCTiming: running";

PetrilloAtWork · 2026-10-12T21:02:50Z

+
+  // Recovers the dk2nu v01_11_00 SetT(0) bug (fixed in v01_12_00) by adding back
+  // the missing meson decay time + tof from decay to flux window.
+  constexpr double kSpeedOfLight = 29.9792458; // cm/ns


I recommend using the existing definition util::kc from larcoreobj/SimpleTypesAndConstants/PhysicalConstants.h (which should be explicitly #included).

PetrilloAtWork · 2026-10-12T21:07:02Z

+    if (std::isnan(nu.prod_time)) continue; // no dk2nu info for this sample/interaction
+    if (nu.dk2gen <= -998.) continue; // simb::MCFlux::fdk2gen unfilled default is -999.
+
+    offset_ns[i] = nu.prod_time + nu.dk2gen * 100. / kSpeedOfLight;


I suggest to use util::kMeterToCentimeter instead of the "magic value" 100. I know it sounds silly, but reading in the code * util::kMeterToCentimeter is much more immediate than * 100.

Suggested change

offset_ns[i] = nu.prod_time + nu.dk2gen * 100. / kSpeedOfLight;

offset_ns[i] = nu.prod_time + nu.dk2gen * util::kMeterToCentimeter / kSpeedOfLight;

PetrilloAtWork · 2026-10-12T21:32:04Z

+    for (SRTrueParticle &part: nu.prim) {
+      if (part.genT   > -9998.) part.genT   += delta_us;
+      if (part.startT > -9998.) part.startT += delta_us;
+      if (part.endT   > -9998.) part.endT   += delta_us;


For your information: I have introduced an helper that should retrieve the default value of a field (in this case the test would look would look like: part.genT != caf::SRDefaults::For(part).genT).
As usual in C++, it is a mouthful but it saves you from putting magic values in the code.
However, that helper is not integrated in the branch you are targeting (and I think not yet in develop either).

PetrilloAtWork · 2026-10-12T21:59:51Z

+  art::Handle<std::vector<bsim::Dk2Nu>> dk2nu_handle;
+  GetByLabelStrict(evt, std::string("generator"), dk2nu_handle);
+
+  std::vector<art::Ptr<bsim::Dk2Nu>> dk2nus;
+  if (dk2nu_handle.isValid()) {
+    art::fill_ptr_vector(dk2nus, dk2nu_handle);
+  }


Do not go through a vector of art pointers unless you need art pointers:

Suggested change

art::Handle<std::vector<bsim::Dk2Nu>> dk2nu_handle;

GetByLabelStrict(evt, std::string("generator"), dk2nu_handle);

std::vector<art::Ptr<bsim::Dk2Nu>> dk2nus;

if (dk2nu_handle.isValid()) {

art::fill_ptr_vector(dk2nus, dk2nu_handle);

}

art::Handle<std::vector<bsim::Dk2Nu>> dk2nu_handle;

GetByLabelStrict(evt, fParams.GenLabel(), dk2nu_handle);

std::vector<bsim::Dk2Nu> const* dk2nus =

dk2nu_handle.isValid()? dk2nu_handle.product(): nullptr;

(dk2nus pointer will be null if the product is not available)

This is a request. I have just added guideline [CL-10] since this is a very widely misused pattern.

PetrilloAtWork · 2026-10-12T22:01:02Z

  for (size_t i=0; i<mctruths.size(); i++) {
    auto const& mctruth = mctruths.at(i);
    const simb::MCFlux &mcflux = (mcfluxes.size()) ? *mcfluxes.at(i) : badflux;
+    const bsim::Dk2Nu &dk2nu = (dk2nus.size()) ? *dk2nus.at(i) : baddk2nu;


Bypassing the art pointers as suggested earlier:

Suggested change

const bsim::Dk2Nu &dk2nu = (dk2nus.size()) ? *dk2nus.at(i) : baddk2nu;

const bsim::Dk2Nu &dk2nu = dk2nus? dk2nus->at(i) : baddk2nu;

PetrilloAtWork · 2026-10-12T22:45:19Z

+      if (ok) {
+        srtruthbranch.nu.back().generator = caf::kGENIE;
+      } else if (i < mevprtl_truths.size()) {
+        srtruthbranch.nu.back().generator = caf::kMeVPrtl;
+      } else {
+        srtruthbranch.nu.back().generator = caf::kUnknownGenerator;
+      }


I suggest to avoid the repetition for maintenance reasons:

Suggested change

if (ok) {

srtruthbranch.nu.back().generator = caf::kGENIE;

} else if (i < mevprtl_truths.size()) {

srtruthbranch.nu.back().generator = caf::kMeVPrtl;

} else {

srtruthbranch.nu.back().generator = caf::kUnknownGenerator;

}

SRTrueInteraction& nu = srtruthbranch.nu.back();

int& generator = nu.generator;

if (ok) generator = caf::kGENIE;

else if (i < mevprtl_truths.size()) generator = caf::kMeVPrtl;

else generator = caf::kUnknownGenerator;

(yes, it would have been better for the rest of the code to avoid that repetition too...)

PetrilloAtWork · 2026-10-12T22:49:15Z

+    std::cout << "CAFMaker: run " << evt.run() << " subRun " << evt.subRun()
+              << " fSubRunPOT=" << fSubRunPOT
+              << " frameApplyAtCaf=" << rec.sbnd_frames.frameApplyAtCaf << std::endl;
+


I suggest to move this output to mf::LogDebug("CAFMaker") or to remove it completely.

PetrilloAtWork · 2026-10-12T23:10:37Z

  }

+  art::Handle<std::vector<bsim::Dk2Nu>> dk2nu_handle;
+  GetByLabelStrict(evt, std::string("generator"), dk2nu_handle);


I request the input tag not to be hard-coded:

Suggested change

GetByLabelStrict(evt, std::string("generator"), dk2nu_handle);

GetByLabelStrict(evt, fParams.GenLabel(), dk2nu_handle);

(there is another line in CAFMaker that uses hard-coded "generator", and it should not either)

VCLanNguyen and others added 4 commits September 25, 2026 12:34

Extend SBND data-stream CRT/PMT reference-time shift to opt0_sec, bar…

a8a5c66

…ycenterFM, fmatch variants, and CRUMBS features Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lxqnx6r8HqPPywzXcxBahz

Fill SRTrueInteraction.prod_time and .dk2gen from dk2nu/MCFlux in CAF…

28ff628

…Maker Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lxqnx6r8HqPPywzXcxBahz

Correct dk2nu v01_11_00 vertex-time bug for SBND MC truth and slice-s…

865a4b1

…coped reco objects Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lxqnx6r8HqPPywzXcxBahz

VCLanNguyen marked this pull request as draft September 25, 2026 17:38

VCLanNguyen added dependent An issue or PR depending on another bugfix Addresses one or more bugs labels Sep 25, 2026

VCLanNguyen changed the title ~~Bugfix gen2 mctiming merge correctedopflash~~ BugFix in CAFMaker for dk2nu v1_11_00 Sep 25, 2026

VCLanNguyen changed the title ~~BugFix in CAFMaker for dk2nu v1_11_00~~ Bug Fix in CAFMaker for dk2nu v1_11_00 Sep 25, 2026

VCLanNguyen mentioned this pull request Sep 25, 2026

Gen2 Production PR: Manual Fix for GENIE timing bug in CAFMaker + Extend existing-data shift in CAFMaker SBNSoftware/sbnanaobj#199

Open

5 tasks done

VCLanNguyen added 2 commits September 25, 2026 12:54

VCLanNguyen assigned VCLanNguyen and jorge-romeo Sep 25, 2026

VCLanNguyen and others added 8 commits September 28, 2026 17:48

Drop redundant flash-match guard comment in FixPMTReferenceTimes

9752c09

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Comment MeVPrtl prod_time fill to match dev tree wording

0831e92

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Add debug print to CorrectMCTiming to confirm it runs

31fad5a

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

VCLanNguyen changed the title ~~Bug Fix in CAFMaker for dk2nu v1_11_00~~ Gen2 Production PR: Sep 30, 2026

VCLanNguyen changed the title ~~Gen2 Production PR:~~ Gen2 Production PR: 1. Manual Fix for GENIE timing bug in CAFMaker, 2. Extend existing-data shift in CAFMaker, 3. Update to SBND LightPropagation Module, 3. Sep 30, 2026

VCLanNguyen mentioned this pull request Sep 30, 2026

Gen2 Production PR: Update to SBND LightPropagation Module SBNSoftware/sbndcode#967

Open

4 of 7 tasks

Add debug print for fSubRunPOT/frameApplyAtCaf gate

c6c16ea

VCLanNguyen marked this pull request as ready for review October 1, 2026 18:55

VCLanNguyen requested a review from PetrilloAtWork October 1, 2026 18:56

VCLanNguyen requested a review from tjones79 October 1, 2026 18:56

tjones79 requested changes Oct 12, 2026

View reviewed changes

PetrilloAtWork requested changes Oct 12, 2026

View reviewed changes


		void CAFMaker::CorrectMCTiming(StandardRecord &rec) const {

		std::cout << "CAFMaker::CorrectMCTiming: running" << std::endl;

	std::cout << "CAFMaker::CorrectMCTiming: running" << std::endl;
	mf::LogDebug("CAFMaker") << "CorrectMCTiming: running";

	offset_ns[i] = nu.prod_time + nu.dk2gen * 100. / kSpeedOfLight;
	offset_ns[i] = nu.prod_time + nu.dk2gen * util::kMeterToCentimeter / kSpeedOfLight;

	const bsim::Dk2Nu &dk2nu = (dk2nus.size()) ? *dk2nus.at(i) : baddk2nu;
	const bsim::Dk2Nu &dk2nu = dk2nus? dk2nus->at(i) : baddk2nu;

	GetByLabelStrict(evt, std::string("generator"), dk2nu_handle);
	GetByLabelStrict(evt, fParams.GenLabel(), dk2nu_handle);

Conversation

VCLanNguyen commented Sep 25, 2026 • edited by PetrilloAtWork Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Validation:

Dependency:

Gen2 Production:

Develop:

Checklist:

Uh oh!

VCLanNguyen commented Oct 7, 2026

Uh oh!

tjones79 left a comment

Choose a reason for hiding this comment

Uh oh!

tjones79 Oct 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

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!

4 participants

VCLanNguyen commented Sep 25, 2026 •

edited by PetrilloAtWork

Loading

tjones79 Oct 12, 2026 •

edited

Loading