EventSelector modifications for new MC waveform hits

[S81D]

Describe your changes

See PR #329 for more details on why we need to alter downstream tools. For the PMTs,EventSelector assumes MC hits are coming from the MC cluster map. For the new MC waveform tool (#324) the ClusterFinder tool handles the MC hits in an identical fashion to the data hits. We therefore need to adjust some of the logic to allow for this tool to use "data" PMT clusters with MC MRD hits, truth information, etc...

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
• [N/A] 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
• [N/A] For every new usage, there is a reason the data must be on the heap
• [N/A] For every new there is a delete, unless I explicitly know why (e.g. ROOT or a BoostStore takes ownership)

Additional Material

This PR (just like #329) is dependent on Andrew's new MC waveform tool (#324).

[jminock]

Looks good! Merge with PMTWaveformSim

[jminock]

Merge with PMTWaveformSim

[brichards64]

Is this merged, ready for level 0 or on hold? can we update the tags so its clear?

[jminock]

It is on hold because it needs to be merged with or after PR #358 Please see above comments

[jminock]

PR #358 has been merged; so this PR is ready for merge

[marc1uk]

this seems like a fairly large block of duplicated code based on

if(data){
  /* do X with data-like hits */
} else {
  if(simulated data){
    /* do X with data-like hits */
  } else {
    /* do X with MC-like hits */
  }
}

can we at the least replace with

if(data || simulated data){ ...

to reduce code duplication. Alternatively if the MC/data manipulation code can be moved into a templated function or lambda taking an auto argument (see this comment), perhaps we could eliminate the duplication entirely...
This would be so much easier if the vectors held pointers to hits... 🙈

[S81D]

I can make those changes and re-commit. It is pretty ugly, my apologies. I just modified the existing for loops as they already existed.

[marc1uk]

the PR overall looks fine, but i'm somewhat uneasy with the amount of code duplication going on.... can we at least reduce it to two copies with consistent if(data || MC_waveforms) throughout?

[S81D]

Yes I can clean it up a bit. I'll add the if(data || MC_waveforms) instead of the nested if statements.

[jminock]

Looks good!

[marc1uk]

Thanks for this, most of the merges look good (and I think i spot some bugfixes too). I can still see one outstanding duplicate: lines 1077-1098 are the same as 1101-1121, and could be merged by updating the check on line 1075.

[S81D]

Thanks for taking a look - I'll modify it and commit the changes

[S81D]

Done! Near line 100, I had previously included a condition that will skip the event if an upstream tool told it to do so (the original point of this PR). It was added to be compatible with the PMTWaveformSim tool, as the tool will skip events with no MCHits (since no waveforms are to be reconstructed). Andrew and I agreed this should be changed at some point moving forward, as skipping entries results in dropping events with 0 hits for cross section analyses (which we don't want!). I will be pushing changes later today that modify the waveform simulator to not skip events. Part of that includes "un-doing" some commits to downstream tools like this one, as there will no longer be any event skipping 😓 To save time I just removed it from this tool.

[marc1uk]

awesome, thanks Steven 🎉