Add LAPPDLoadStore tool for EventBuildingV2 tool chain #333
Conversation
The LAPPDHit and LAPPDPulse.h are for LAPPD so they are included in this PR
jminock
left a comment
jminock
left a comment
There was a problem hiding this comment.
No major issues, and I believe this is currently in use so it has been tested and works
jminock
added
Ready for Level 0 Review and merge
and removed
Ready for Software meeting discussion
labels
| LAPPDana = LoadData(); | ||
| m_data->CStore.Set("LAPPDana", LAPPDana); | ||
| if (LAPPDStoreReadInVerbosity > 0) | ||
| cout << "LAPPDana for loading was set to " << LAPPDana << endl; |
There was a problem hiding this comment.
is this a warning? if not, increase printout verbosity. If it is, perhaps the printout could be clearer as to what the problem may be...
| LAPPDana = false; | ||
| m_data->CStore.Set("LAPPDana", LAPPDana); | ||
| m_data->CStore.Set("LAPPDPPShere", LAPPDana); | ||
| return true; |
There was a problem hiding this comment.
on error return false allows users to control whether to stop the toolchain with the ToolChainConfig errorlevel parameter.
| m_data->Stores["ANNIEEvent"]->Set("LAPPD_ID", LAPPD_ID); | ||
| m_data->CStore.Set("LoadingPPS", true); | ||
| if (LAPPDStoreReadInVerbosity > 0) | ||
| cout << "LAPPDStoreReadIn: PPS data loaded, LAPPDanaData is false, set LAPPDana to false" << endl; |
There was a problem hiding this comment.
again is this a warning level printout?
|
|
||
| if (!parsData) | ||
| { | ||
| cout << "LAPPDStoreReadIn: PSEC data parsing failed, set LAPPDana to false and return" << endl; |
There was a problem hiding this comment.
if something failed, return false
| vector<int> ACDCReadedLAPPDID; | ||
|
|
||
| if (LAPPDStoreReadInVerbosity > 0) | ||
| cout << "LAPPDStoreReadIn: LAPPDDataMap has " << LAPPDDataMap.size() << " LAPPD PSEC data " << endl; |
There was a problem hiding this comment.
is this a warning? else increase verbosity
| { | ||
| if (BGCorrections.find(key) == BGCorrections.end()) | ||
| { | ||
| std::cerr << "Error: Key not found in BGCorrections: " << key << std::endl; |
There was a problem hiding this comment.
if there is a possibility of error this method should probably be able to return a status and have that checked...
| std::map<string, vector<int>> TSCorrections; //TS corrections, in unit of ticks | ||
| */ | ||
|
|
||
| TFile *file = new TFile("offsetFitResult.root", "READ"); |
There was a problem hiding this comment.
hard-coded filepath? Is this file always necessarily present?
prefer config variable, should check file.IsZombie()
| if (!tree) | ||
| { | ||
| std::cerr << "LAPPDStoreReadIn Loading offsets, Tree not found!" << std::endl; | ||
| return; |
There was a problem hiding this comment.
method should provide return value that should be checked
| #include "TFile.h" | ||
| #include "TTree.h" | ||
|
|
||
| #define NUM_CH 30 |
There was a problem hiding this comment.
I believe these will collide with other tools and prevent issues with the new Makefile.
Change to constexpr or put these in DataModel/ANNIEConstants.h as they're #define'd in many Tools
| #define NUM_PSEC 5 | ||
| #define NUM_SAMP 256 | ||
|
|
||
| using namespace std; |
|
A serious question, could we merge them first, then edit the exist code in the main repo? Because we have had this list for preferred changes. If we merge them first, then I can PR the latest changes I have on the EBV2Freeze branch, which is quite small, and then I can modify your comments accordingly then run one complete test. |
|
sure. I think a few of these ought to be done, while most are nice-to-haves. |
3 participants
LAPPDLoadStore Tool in EventBuildingV2 tool set.
This is also splited from PR #307
Changes based on your comments:
· LAPPDLoadStore around line 674/705 looks similar code to that on the monitoring toolchain which crashes and has to be worked around. There are some try/catches here too - does this throw? Perhaps these were added at my request to investigate - did you compare what this is doing with the online monitoring code and see if there are differences that may explain why the monitoring code doesn't like it?
Now it's at line 675.
I remember an error about these code last year when I started to rewrite LAPPD tools. Sometime, for some reason I don't understand (maybe related to the firmware or something before the saving in DAQ), the meta data may miss something so the length is not enough for the code to load until INFOWORD == 13 or TRIGGERWORD == 6. This happens very rare, even less than 1 event per run. But it should crash any tool chain related to LAPPD data. Now I add try/catch for that and say if there is such an error just skip that event in processing, but for old tools this might be useful. I will have a look for the monitoring tool chain maybe later next week.
· I see a new without delete - in fact is there any reason PedestalValues needs to be on the heap at all? As far as i can tell it could just be a member vector not a pointer.
There is not a specific reason, I just keep the convention from old LAPPD tools. Since everything is working fine I would perfer not to change......