Update LAPPD ID handling to support LAPPD INCOM IDs#381
Update LAPPD ID handling to support LAPPD INCOM IDs#381anuj-guptta wants to merge 3 commits intoANNIEsoft:Applicationfrom
Conversation
Motivation: Newer runs use Manufacturer (Incom) IDs instead of ACC IDs, which breaks compatibility with existing downstream tools expecting ACC IDs. Changes: - Added a structure in DataModel/PsecData.h to store LAPPD ID mapping information loaded from configfiles/LAPPDProcessedAna/LAPPDIDConfig.csv in UserTools/LAPPDLoadStore/LAPPDLoadStore.cpp. - Implemented lookup logic to retrieve the appropriate mapping for a given run number and ACC ID, returning the corresponding Manufacturer ID and position. - Added reverse mapping function to convert Manufacturer IDs back to ACC IDs for compatibility with downstream tools. - Removed redundant structure previously defined in ANNIEEventTreeMaker.h including some fixes. Heuristic: - If LAPPD_ID > 20, it is treated as a Manufacturer (Incom) ID (based on current detector configuration assumptions). Notes / Limitations: - Current implementation relies on a heuristic (ID > 20) to distinguish ID types. - This assumption is based on current detector configuration and observed ID ranges. - Planned improvement: replace heuristic with run-dependent or database-driven mapping. - Mapping file (LAPPDIDConfig.csv) must be updated as detector configuration evolves. Validation: - Basic functionality verified (toolchain runs end-to-end). - Detailed validation of Event Building + data processing before-and-after is pending.
- Update ID mapping logic for consistent handling across the toolchain. - Store Pulse IDs and Hit IDs as INCOM IDs.
| result= queryNearestACCID(idConfigRecords, testRunNum, testManuID); | ||
| int nearestACCID = std::get<0>(result); | ||
| position = std::get<1>(result); | ||
| cout << "Querying nearest ACCID for RunNumber: " << testRunNum << ", ManufacturerID: " << testManuID << endl; |
There was a problem hiding this comment.
You can put in a verbosity condition here (>1 or something equivalent)
There was a problem hiding this comment.
There is already a verbose level assigned to this in line 227.
| int testManuID = 39; // example ManufacturerID to query | ||
| auto result= queryNearestACCID(idConfigRecords, testRunNum, testManuID); | ||
| int nearestACCID = std::get<0>(result); | ||
| std::string position = std::get<1>(result); |
There was a problem hiding this comment.
same thing here - add a verbosity threshold
There was a problem hiding this comment.
There is a verbose level here as well in line 157 but I will increase it to '1'.
|
This is great to see you've added the LAPPD ID and position mapping, this will be super helpful! I also appreciate the detailed description on the individual commits, the validation testing, and the note on limitations / future work. For future PRs and ease of readability I would recommend just adding those comments to the main PR and also filling out the checklist. No obvious problems stand out to me. You note that the "detailed validation of Event Building + data processing before-and-after is pending" - once that is verified I think its good to go and I will give my stamp of approval. |
S81D
added
Ready for Level 0 Review and merge
waiting for submitter
and removed
Ready for Level 0 Review and merge
labels
|
Thanks, Steven, for reviewing this. I don’t want to take credit for the original work, most of it was done by Yue last year but wasn’t merged into the latest ToolAnalysis release. I’ve applied a minimal version of his changes along with few modifications to get a working EventBuildingV2 and associated toolchains. For validation of all data processing steps, I’ve tested three part-files, and everything works as expected. The LAPPD data before and after these changes remains unchanged. Let me know if this is enough for the approval. |
|
Great! I will mark it as ready for level 0 review and merging. |
2 participants
Describe your changes
Checklist before submitting your PR
newusage, there is a reason the data must be on the heapnewthere is adelete, 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.