Skip to content

SBND Timing Reconstruction Refactor#915

Open
VCLanNguyen wants to merge 69 commits intodevelopfrom
feature/frame_shift_refactor_v10_14_02
Open

SBND Timing Reconstruction Refactor#915
VCLanNguyen wants to merge 69 commits intodevelopfrom
feature/frame_shift_refactor_v10_14_02

Conversation

@VCLanNguyen
Copy link
Copy Markdown
Contributor

@VCLanNguyen VCLanNguyen commented Feb 14, 2026

Description

  • Timing reconstruction is refactored in this PR, affecting the decode/reconstruction workflow of CRT/PMT/XA.
  • After the refactoring, the @FrameShiftModule@ runs first in the decode chain, outputs timing products to be used at PMT/XA decoder and CRTStrip reconstruction.
  • Relevant PMT reconstruction modules at Reco1/Reco2 are updated.
  • The timing correction applied at CAFMakeer is undone. - The timing correction applied at CAFMakeer is undone. This should ressolve this issue: Converge timing correction in CAFMaker for SBND and ICARUS sbncode#567
  • Details and validation plots can before in the linked docdb.

Link(s) to docdb describing changes (optional)

https://sbn-docdb.fnal.gov/cgi-bin/sso/ShowDocument?docid=46654

Relevant PR links (optional)

This PR needs the XA decoder PR in sbndcode to go in first:
#847

This PR needs to be merged together with this group of PRs:

Checklist

  • Added at least 1 label from available labels.
  • Assigned at least 1 reviewer under Reviewers,
  • Assigned all contributers including yourself under Assignees
  • Linked any relevant issues under Developement
  • Does this PR affect CAF data format? If so, please assign a CAF maintainer (PetrilloAtWork or JosiePaton) as additional reviewer.
  • Does this affect the standard workflow?
  • Is this PR a patch for the ongoing production? If so, separate PR must also be made for production/v10_06_00 branch!

aliciavr and others added 30 commits October 9, 2025 04:05
@aantonakis
Copy link
Copy Markdown
Contributor

crt veto looks good

Copy link
Copy Markdown
Contributor

@lynnt20 lynnt20 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved for PMT side!

Copy link
Copy Markdown
Contributor

@aliciavr aliciavr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good on XA's side!

@VCLanNguyen VCLanNguyen requested a review from JosiePaton April 20, 2026 19:37
@VCLanNguyen
Copy link
Copy Markdown
Contributor Author

@JosiePaton @PetrilloAtWork can you guys review for CAF?

Copy link
Copy Markdown
Member

@PetrilloAtWork PetrilloAtWork left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some changes suggested here and there as usual, plus:

  1. something that looks like a bug (which must be fixed, hence the rejection)
  2. a questionable implementation of the difference of timestamps

A reiterated comment/recommendation: please format all the relevant comments on the usage of the functions and data member to Doxygen format.

Comment thread sbndcode/CRT/CRTReco/CRTStripHitProducer_module.cc
Comment thread sbndcode/CRT/CRTReco/CRTStripHitProducer_module.cc Outdated
Comment thread sbndcode/CRT/CRTReco/CRTStripHitProducer_module.cc Outdated
Comment thread sbndcode/CRT/CRTReco/CRTStripHitProducer_module.cc Outdated
Comment thread sbndcode/CRT/CRTReco/CRTStripHitProducer_module.cc Outdated
Comment thread sbndcode/Timing/FrameShift/FrameShift_module.cc Outdated
Comment thread sbndcode/Timing/WaveformAlignment/WaveformAlignment_module.cc Outdated
Comment thread sbndcode/Timing/CMakeLists.txt Outdated
Comment thread sbndcode/Timing/TimingUtils.h Outdated
Comment thread sbndcode/Timing/TimingUtils.h Outdated
@VCLanNguyen
Copy link
Copy Markdown
Contributor Author

Hi @PetrilloAtWork , I resolved all comments except for 1 remaining that I'm sending to the author of the code.

Can you clarify if the two statements below are part of the comments that I have resolved, or they are separate and I have missed them. Thank you

  1. something that looks like a bug (which must be fixed, hence the rejection)
    
  2. a questionable implementation of the difference of timestamps
    

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change caf Common analysis framework crt Cosmic Ray Tagger data features for data processing pds Photon Detection System

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants