Add ANNIEEventTreeMaker as the part 2 of EBV2, replace PhaseIITreeMaker

[fengyvoid]

ANNIEEventTreeMaker Tool in EventBuildingV2 working flow but not in the EBV2 tool chain. This should replace PhaseIITreeMaker and give much cleaner behavior based on the new built events. (Each entry in the tree means one event)

This is splitted from PR #307.

Changes based on your comments:

· Can we get these from input file?
m_variables.Get("isData", isData);
m_variables.Get("HasGenie", hasGenie);
I think you are right that if it can directly detect it's data file or Genie file, but unfortunately I don't know such a way. To make it better, there is a BeamClusterAnalysis tool chain and another BeamClusterAnalysisMC tool chain, and these configs are different in them.

· Does the closing } at line 486 need to be moved to 479? shouldn't cuts be applied even if fillCleanEventsOnly is true, for clean events?
We want to include everything rather than only fill clean events. In EBV2, all groups of trigger will be in the processed data, and all of them will go to the root tree in this tool, this will help to understand what is in the file. For example in docdb-5494 page 9, we can see how many PMT hits in all triggers in this run and notice that problem. If anyone want to do pure physics analysis they can select only clean events.

· line 970 - replace cout usage.
Done, but I am not sure what's the difference between Log and cout? Maybe Log is faster? I see some tools use Log and some tools use cout. I guess Log may can generate a actual log file base on config of ToolChainConfig?

· Why is there a Get into an unsigned int then static cast into int? BoostStore::Get uses the streamer operator>>, should be fine to populate a signed integer directly. This is done a few times throughout the code.
If you mean at line 1021, I think in principle you are right, but I just explicitly write it there

· Line 988, 993 - can just use fTankMRDCoinc = pmtmrdcoinc for each ot these. The compiler will implicitly cast bool to int.
Now at line 1039, thanks, changed.

· Line 1023 seems redundant (done by ResetVariables call).
Removed.

· Line 1136 - here and several other places, i don't know why we do this:

     unsigned long channel_key_data = channel_key;
    if (!isData)
    {
      int wcsimid = channelkey_to_pmtid.at(channel_key);
      channel_key_data = pmtid_to_channelkey[wcsimid];
    }

Very good question, I copied them from PhaseIITreeMaker but didn't check them because the whole block works. Maybe pmtid_to_channelkey in wcsim is different then data? I know LAPPD channel keys are surely different.

· ANNIEEventTreeMaker.cpp lines 1114 to 1189, and a bunch of other places where data and MC are being handled essentially identically by two different blocks of code, etc.
Very good tricks, but change all of them will cost some time to update and debug. Could we leave this to the future? Curent version works well without any problem.

· Line 1731 - if this is a fatal error (genuinely only if a user has not included a necessary Tool) would be good to set m_data->vars.Set("stop_loop",1) to terminate the toolchain. Printouts can often be missed.
Good point, added. Btw it's m_data->vars.Set("StopLoop",1);

· Feels like the checks and corrections at 1974 should not be in this Tool - should be part of the reconstruction...
Not disagree, but I don't know is there any convention for those in analysis part. Maybe this can be a furutre update?

Thanks a lot for all the comments!

[fengyvoid]

This build requires the LAPPDHit.h and LAPPDPulse.h, which were included in previous PR #333, so the compile fails

[jminock]

Looks good! The new Tool shares a lot of code from PhaseIITreeMaker. Given the code from that Tool is acceptable with no major problems, and this Tool would be at the end of a ToolChain, I find no problem with merging. Given the similarity between this Tool and PhaseIITreeMaker and this Tool's more accessible ntuple file, I think down the road, those managing ToolAnalysis should make a decision if PhaseIITreeMaker would still be necessary to keep in ToolAnalysis.

[brichards64]

these all appear to be leaking

[marc1uk]

I think it would be sufficient to simply not put them on the heap

[marc1uk]

there are 304 TTree::Branch calls in this Tool. Makes you wonder if we could design a Storage class that didn't need to define up-front everything that went into it... 🙃