[anuj-guptta]

Describe your changes

This PR is submitted by me on behalf of Amala (BES changes) + me (other minor fixes).

Adds the Beam Extraction Signal (BES) raw waveform to event building, in the same way as the existing RWM and BRF waveforms, and saves it per event in the ANNIEEvent as BESRawWaveform.

Changes:

• PMTDataDecoder

  • Save BES raw waveform (crate 1, slot 3, ch 1) for runs >= 5870, controlled by saveBESRaw (default true).
  • Skip a record header that is split across two frames instead of reading past the end of the frame (that waveform is lost).
  • Fix channel comments and add a TODO: BRF channel for runs 5870-5914 is not handled yet.

• EBPMT

  • Get BESRawWaveforms from the CStore (saveBESWaveforms, default true), and apply VME offset correction.
  • Remove unused variables --> FinishedRWMWaveforms, FinishedBRFWaveforms, RWMEmplacedTimes, BRFEmplacedTimes.

• EBSaver

  • Save BES raw waveform per event, controlled by saveRawBESWaveform (default true); empty vector if not found.
  • Set DataStreams Tank/MRD/LAPPD only when the data was actually saved.
  • Save empty RWM/BRF/BES waveforms in BuildEmptyPMTData() so every event has these keys.
  • Store ConfigInfo (commit and config) in the ANNIEEvent header.

• FitRWMWaveform

  • save BES raw waveforms to the output ROOT file

• PhaseIIADCCalibrator

  • BES is not in AuxChannels.csv, so PhaseIIADCCalibrator skips it (log level of that message lowered to debug) in EventBuilding off mode.
  • The EventBuilding on mode still prints the log.

Notes:

• No BES fit is included yet; only the raw waveform is saved. (to be updated in the near future)
• The non-BES related changes are updated in this document: https://annie-docdb.fnal.gov/cgi-bin/sso/ShowDocument?docid=7024

Additional fixes after review comments

1. Update AuxChannels.csv: add BES and spare slot-3 channels

   • Add crate 1, slot 3, channels 1-4 (BES and three spares) to AuxChannels.csv, so they are mapped Aux channels.
   • This stops the per-event "Cannot find channel key" messages for these channels in runs >= 5870.
   • Restore v_error for unmapped channels in PhaseIIADCCalibrator, so a real missing channel mapping is reported again.
   • Lower verbose in FitRWMWaveformConfig.

2. Update FitRWMWaveform

   • Clear RWM/BRF/BES before Get
   • Skip empty waveforms when writing histograms

3. Update ProcessedLAPPDFilter: copy BESRawWaveform to filtered output

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.

[S81D]

I am good to sign off on this - great to see that we have the BES stored now, thanks!

I attached the Claude review (nothing it flagged should prevent merging, but just some things to check)

review_397.md

[anuj-guptta]

I am good to sign off on this - great to see that we have the BES stored now, thanks!

I attached the Claude review (nothing it flagged should prevent merging, but just some things to check)

review_397.md

Thanks @S81D for the comments. I went through the document, and there were some really good points. Minor items that don’t affect the data are kept for a later cleanup PR. I’ve added 3 commits for the rest (also listed in the PR description):

1. Update AuxChannels.csv: add BES and spare slot-3 channels

   • Add crate 1, slot 3, channels 1-4 (BES and three spares) to AuxChannels.csv, so they are mapped Aux channels.
   • This stops the per-event "Cannot find channel key" messages for these channels in runs >= 5870.
   • Restore v_error for unmapped channels in PhaseIIADCCalibrator, so a real missing channel mapping is reported again.
   • Lower verbose in FitRWMWaveformConfig.

2. Update FitRWMWaveform

   • Clear RWM/BRF/BES before Get
   • Skip empty waveforms when writing histograms

3. Update ProcessedLAPPDFilter: copy BESRawWaveform to filtered output

[S81D]

Nice! thanks for doing this

[anuj-guptta]

Fixed the two points mentioned above.

[marc1uk]

The docdb slides you attached on going over claude's findings are really good, thanks Anuj.
I get the feeling most of them are addressed, but it seems like some are treated as edge cases for "well, we don't use this configuration, so it doesn't affect us".
It would be really great to keep a list of anything outstanding as a set of "known issues" somewhere, so that we don't lose track of those things. I see you've said

Minor items that don’t affect the data are kept for a later cleanup PR.

Is that a public list? I suppose we could use github issues, but i'm not sure that's the best place tbh.

I guess (1) in steven's review of this PR is considered "doesn't affect us" because we're using EBSaver which deletes those waveforms, but if we have a code path with a known catastrophic memory leak like this, we shouldn't lose track of that. In fact generally the CStore is never saved to file, so there's really no point in ever having persist=true for a pointer. It would seem sensible to go through and check all the CStore::Set calls using pointers have persist=false.

The channel mapping not being consistent, but also not having a path in code to alter it based on run number, is really quite bad. At minimum we could add config variables to override things, but i think it would be better to alter our channel mapping files to centrally keep track of this and automatically pick the right one based on run number. Otherwise it's just going to cause headaches. (also as per claude's magic numbers remark, these shouldn't be hard-coded).

Claude's first two Minor checklist items are worth implementing.

[anuj-guptta]

edge cases for "well, we don't use this configuration, so it doesn't affect us".
It would be really great to keep a list of anything outstanding as a set of "known issues" somewhere, so that we don't lose track of those things.

I think best way is go forward as you suggested "Make a list of known issues, upload in docdb and also inform in the collaboration". I will try to write up a note on the known issues in the near future and share with all.

[anuj-guptta]

Minor items that don’t affect the data are kept for a later cleanup PR.

Is that a public list? I suppose we could use github issues, but i'm not sure that's the best place tbh.

These are majorly cout/cerr statements, updating READMEs, etc. which will be taken care by me sooner. I will include this in my "Known Issues" list.

[S81D]

Please also add it to the issues list

[anuj-guptta]

I guess (1) in steven's review of this PR is considered "doesn't affect us" because we're using EBSaver which deletes those waveforms, but if we have a code path with a known catastrophic memory leak like this, we shouldn't lose track of that. In fact generally the CStore is never saved to file, so there's really no point in ever having persist=true for a pointer. It would seem sensible to go through and check all the CStore::Set calls using pointers have persist=false.

Agreed. EB is only safe today because EBSaver deletes those waveforms, which isn't a good reason to leave the leak in place. I will add it to the known-issues list so it doesn't get lost.

[anuj-guptta]

Please also add it to the issues list

Thanks Steven. I will do that.

[anuj-guptta]

The channel mapping not being consistent, but also not having a path in code to alter it based on run number, is really quite bad. At minimum we could add config variables to override things, but i think it would be better to alter our channel mapping files to centrally keep track of this and automatically pick the right one based on run number. Otherwise it's just going to cause headaches. (also as per claude's magic numbers remark, these shouldn't be hard-coded).

Yeah, It was giving me headaches too. I think I will create two .csv files, run dependent and then load these files accordingly (in right order) in LoadGeometry. But we should be good for now.

[anuj-guptta]

Sorry, closed by mistake. );

[anuj-guptta]

Claude's first two Minor checklist items are worth implementing.

Agreed, both are on the known-issues list.

1. BESRawWaveforms: initialise to nullptr in EBPMT.h and EBSaver.h and check the Get result (EBSaver already has an unused gotBES for this). Small and safe, I'll include it in the cleanup PR.
2. operator[]: I’ll switch it to emplace + std::move in the same cleanup PR. I’ll also check whether the overwrite actually happens in our data.