[anuj-guptta]

Describe your changes

Limitations of existing framework

• Metadata read from both ACDCs in LAPPDLoadStore.cpp, but beamgate and timestamp were constructed out of ACDC 0 metadata only.
• This resulted in incorrect beamgate alignment in the offsetFit_MultipleLAPPD.cpp, such that only the ACDC 0 fit was correctly aligned.
  • Opened a PR for updated code: Update offsetFit_MultipleLAPPD.cpp to support dual-ACDC EBV2 ANNIE_HelperScripts#2
• Consequently, only ACDC 0 offsets and corrections were propagated to downstream tools for event matching.
• The existing framework therefore did not recover as many LAPPD events as expected, despite moving towards a PPS pre-scale of 5.

Updates to the EventBuilderV2 framework

• Updated the event-building chain to support the dual-ACDC architecture.
• Restructured ACDC-dependent variables so that quantities associated with the two ACDCs are explicitly separated into ACDC 0 and ACDC 1 variables.
• This version is now dual-ACDC EBV2.
• Updated the tools to accommodate both ACDCs throughout the event-building chain:
  • LAPPDLoadStore, LAPPDTreeMaker, EBLAPPD, EBSaver, ProcessedLAPPDFilter and ANNIEEventTreeMaker.
  • Other tools include PrintDQ and PhaseIITreeMaker.
  • Updated the matching logic in EBLAPPD to use an OR condition between the corrected timestamps from the two ACDCs, allowing LAPPD events associated with either ACDC to be recovered.
• Updated LAPPDLoadStore to support legacy single-board data files that do not contain explicit ACDC board identifiers.
  • Implemented fallback logic to identify and process legacy single-board data.
  • Legacy data are safely routed to downstream tools.
  • ACDC 1 variables are set to zero by default when no ACDC 1 information is available.
• Added LAPPD-related metric to PrintDQ tool.
  • Implemented OR logic for the PPS-missing check when both ACDCs are present.
  • Backward compatibility for legacy single-board data was dropped, since these data quality checks are primarily intended for newly acquired data.

Additional updates

• Cleaned up verbose logging and aligned the verbosity variable name with the tool name in LAPPDLoadStoreConfig.
• Cleaned up ANNIEEventTreeMakerConfig.

Validation

• The updated framework was validated using both beam and laser data.
• Validation was performed on both GPVMs and grid submissions.
• LAPPDTreeMaker and PhaseIITreeMaker tools have limited testing since they are not directly related to data processing stages.

Checklist before submitting your PR

• This PR implements a single change (one new/modified Tool, or a set of changes to implement one new/modified feature)
• This PR alters the minimum number of files to affect this change
• If this PR includes a new Tool, a README and minimal demonstration ToolChain is provided
• If a new Tool/ToolChain requires model or configuration files, their paths are not hard-coded, and means of generating those files is described in the readme, with examples provided on /pnfs/annie/persistent
• For every new usage, there is a reason the data must be on the heap
• For every new there is a delete, unless I explicitly know why (e.g. ROOT or a BoostStore takes ownership)

Additional Material

Attach any validation or demonstration files here. You may also link to relavant docdb articles.

Here are my presentation slides:
https://annie-docdb.fnal.gov/cgi-bin/sso/ShowDocument?docid=6990

[anuj-guptta]

Added my presentation slides in the above described changes for validation.

[marc1uk]

well, i can't see anything amiss, although with 4,000+ lines changed in our critical Tools, perhaps we should try one of those newfangled AI reviews Steven made on it.

What I do see is boilerplate. So much boilerplate.

This is not specifically an issue with this PR in particular, but it touches relevant Tools and highlights a common issue throughout ToolAnalysis - large swathes of code doing the same operation on large numbers of variables:

• ANNIEEventTreeMaker, lines in the 200's, 800s, 1100s, 1400s and 1500s....
• EBLAPPD lines <100, ~100s, 200s, 300s
• EBSaver lines 900s, 1000s, 1200s, 1300s, 1400s
• LAPPDLoadStore lines 100s, 200s, 600s, 1600s, 1700s, 1800s,
• LAPPDTreeMaker lines 0-200s, 500s, 600s, 800s, 1200s,
• PhaseIITreeMaker ... like, all of it...
• ProcessedLAPPDFilter lines 600s, 700s,.....
  and I'm sure there are many, many more throught the repo, but they all look like:

myvar.dothing();
myvar2.dothing();
myvar3.dothing();
myvar4.dothing();
...

or

Store.Get("myvar1",myvar1);
Store.Get("myvar2",myvar2);
Store.Get("myvar3",myvar3);
Store.Get("myvar4",myvar4);
....

or similar. Monolithic blocks, multiple pages long, for simple fetch, set, clear, or the like.
May I introduce: ✨ classes ✨
Package all 20 variables related to one thing (e.g. LAPPD ACDC timing on one side) into a class. Put it in the DataModel. Give it a Reset method to handle resetting everything inside it. Store that class in the ROOT tree as a single branch object, or if people really insist on flat files (for some reason, although your downstream code is subsequently going to have this same problem), give it methods for MakeBranches, FillBranches, FetchBranches and so on that takes a Tree and does those boilerplate tasks. Do the same for putting into and out of (Boost)Stores.

It'll collapse huge amounts of code, make things clearer and easier to maintain, and the code will be so much nicer to review. :)

[marc1uk]

just waiting on @S81D's RoboReview 🤖

[S81D]

I can run it tonight! I need my tokens for another project at the moment :)

[S81D]

https://claude.ai/artifact/EQJ46RnVVy3KhLKAWoppBf#d07467e2-8c49 @anuj-guptta

[anuj-guptta]

https://claude.ai/artifact/EQJ46RnVVy3KhLKAWoppBf#d07467e2-8c49 @anuj-guptta

Thanks @S81D , I will take a look by tomorrow and get back to this PR.

[marc1uk]

i can't look at it without logging in. 🥲

[S81D]

Sorry @marc1uk ! - find attached a PDF of the artifact
ToolAnalysis PR #393 review.pdf

[marc1uk]

thanks steven.
Looks like you've got your work cut out for you Anuj. 😅
Incidentally the report says LAPPDTreeMaker used to fill twice for every event, but doesn't give a code reference... that seems like a fairly significant bug that i'd have expected to have been seen. Is that true? I can't spot in the diffs anything that looks like it's removing a duplicate fill...

I strongly agree with the remarks in "Design questions for the author" about Data Structure - it's similar to my previous remarks about using classes, in a more narrowly scoped but much more pointed way. 34 CStore buffers, 32 ANNIEEvent maps and parallel tree branches, all being duplicated. Oof!
Bullet 2 is also very prudent.

Please do review the whole document; e.g. the multiplicative factor to Correction mentioned in bullet 2 and the shift in beam gate (perhaps that's intended?) mentioned in bullet 4 on page 7 could be insidious bugs if they aren't intended.
There are many other good minor remarks, e.g. use of cout/cerr.

[anuj-guptta]

hii @marc1uk , @S81D , Sharing my responses to the review here:
https://docs.google.com/document/d/1Nsduc1IzATKjAmld9kHQJvNyyNnXTPA-fZnaRqr42Bg/edit?tab=t.0

Most of the points flagged in the Claude review either don’t affect our processing or can be left for a follow-up cleanup PR. I’ve fixed the minor issues that needed immediate attention.

[anuj-guptta]

Incidentally the report says LAPPDTreeMaker used to fill twice for every event, but doesn't give a code reference... that seems like a fairly significant bug that i'd have expected to have been seen. Is that true? I can't spot in the diffs anything that looks like it's removing a duplicate fill...

The LAPPD_IDs.push_back was used twice for the same event, one in Execute() and another in LoadLAPPDMapInfo(). Removed in Execute().

[anuj-guptta]

the multiplicative factor to Correction mentioned in bullet 2

@marc1uk , this was definitely a bug. This correction comes from offset fitting output loaded in LAPPDLoadStore in units of time ticks and then applied in EBLAPPD. There was a unit mismatch back then, corrected in the PR.

[anuj-guptta]

the shift in beam gate (perhaps that's intended?) mentioned in bullet 4

I’m not sure what the original intention behind the drift correction was, but it doesn’t affect our results. TS_driftCorrection_ns is only non-zero when a PPS interval drifts by more than 2 µs, which doesn’t happen in our data normally, so it’s zero in almost all cases. If an interval did drift that far, its timing wouldn’t be precise enough for us anyway, and those events are removed by the timing cuts applied downstream. Either way, the correction is far smaller than the 400 µs matching window in EBLAPPD, so it doesn’t change event building.

I hope I have responded to everything and fixed that was needed in this PR. Let me know if there is anything more required from me.

[marc1uk]

The LAPPD_IDs.push_back was used twice for the same event, one in Execute() and another in LoadLAPPDMapInfo(). Removed in Execute().

i'm surprised that went unnoticed.

Thanks for the thorough review; looks like all the critical parts on the current analysis path are addressed.

I do grow concerned that our codebase is accumulating an increasing amount of legacy code with known breakages.... feels like we're reaping the results of accumulated poor practices in terms of code factoring and separation of concerns, and a complete overhaul is now the only way to get back on track.
If old code is known not to work, perhaps it ought to be removed. It would still exist in git history...

In any case, as commented in the other PR, please document outstanding points being left for later.