SBND Timing Reconstruction Refactor

[VCLanNguyen]

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. 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:
SBNSoftware/sbndcode#847

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

• sbndcode: SBND Timing Reconstruction Refactor sbndcode#915
• sbncode: SBND Timing Reconstruction Refactor sbncode#637
• sbnobj: SBND Timing Reconstruction Refactor #170
• sbnanaobj:SBND Timing Reconstruction Refactor sbnanaobj#188

[VCLanNguyen]

@JosiePaton @PetrilloAtWork can you guys review for CAF?

[PetrilloAtWork]

This is a very strong breaking change, which can be focussed into two places.

1. sbnd::timing::FrameShiftInfo: I think it's fair to say it's a completely different class, at the point where one should think what happens when this new class is initialised with old data. ROOT will convert what it can, and from the documentation it seems the conversion will be wrong (turning from a shift — double — to a timestamp — uint64_t). I strongly recommend you to think about this issue, and act accordingly. Two possibilities that come to my mind are:
   1. Remove version 10 from classes_def.xml; I am not sure which type of failure this will trigger when reading old data, but I believe there will be some. This would need to be verified.
   2. Write explicit conversion code ("ROOT I/O rule") handling the conversion. In the minimal case, this rule would throw an exception claiming the "upgrade" is not supported.
2. sbnd::timing::TimingInfo: undefined values change from 0 to something like INT_MAX. Again old data will be interpreted differently. An I/O rule changing the data members that are 0 to the new undefined value should be arranged, and the code should be surveyed to make sure that the magic undefined value 0 is not implicitly used.

[PetrilloAtWork]

A note, maybe just for the future: this class has private data accessible only via member functions, which means that users will have to interact with the latter; but only the former are documented inline, which makes one additional step (involving parsing source code) to get to the documentation.
I would suggest the interface to be documented with priority over the internal implementation (data in this case); ideally, both, although it adds maintenance burden.

[VCLanNguyen]

This is a very strong breaking change, which can be focussed into two places.

1. sbnd::timing::FrameShiftInfo: I think it's fair to say it's a completely different class, at the point where one should think what happens when this new class is initialised with old data. ROOT will convert what it can, and from the documentation it seems the conversion will be wrong (turning from a shift — double — to a timestamp — uint64_t).

I agree that this is a break changing for SBND workflow. We refactored the entire decoding on raw data to unify the timing reconstruction. Understandably, this is a clean up attempt of bad code writing in the first place.
So initialising this new class with 'old decoded data' should break.

However, I don't fully understand all of your suggestions below, so adding my question to each your point below.

1. Remove version 10 from `classes_def.xml`; I am not sure which type of failure this will trigger when reading old data, but I believe there will be some. This would need to be verified.

Is the suggestion to keep or remove this line?

2. Write explicit conversion code ("ROOT I/O rule") handling the conversion. In the minimal case, this rule would throw an exception claiming the "upgrade" is not supported.

Agree that this can be a clean suggestion although I do not know what ROOT I/O rules are.

• Can you point me to an example?
• Where does the I/O need to be implemented, for example, in the module where the classes are created?

2. sbnd::timing::TimingInfo: undefined values change from 0 to something like INT_MAX. Again old data will be interpreted differently. An I/O rule changing the data members that are 0 to the new undefined value should be arranged, and the code should be surveyed to make sure that the magic undefined value 0 is not implicitly used.

• So in the module, the classes are initialised like this so 0 is not explicitly called anywhere. Is that sufficient?