LoadGenieEvent Update

[jminock]

Updated reading in GENIE files from WCSim and standalone. Includes more flux information necessary for flux systematic reweighting Includes more final state lepton information such as PDG, energy, momentum, initial vertex, time of initial vertex, and mass Includes number of charged final state particles (pi+, pi-, K+, K-) that meet Cherenkov threshold Some cleaning

[marc1uk]

Ok, I can't spot anything that looks like a major hangup here. Couple minor questions though:

Firsly it feels to me like fsleptonmom (and similar) should be a Direction class instance rather than a Position... i realise both probably do the job, but a direction seems more appropriate for a momentum?

Secondly: i noticed you removed this section of code that only loads the next Genie event if this is a not a delayed WCSim trigger, and I'm wondering why. As far as I can tell, when loadwcsimsource is false this Tool will assume 1:1 matching between Genie file entries and WCSim file entries, but if the LoadWCSim tool is operating in a mode where it returns subtriggers as separate entries, then this would cause the two Tools to get out of sync. Is there a reason this was removed?

Finally, your PR fails the CI checks, as it fails to compile. Please check the build logs.

[jminock]

Thanks for taking a look! I'll go in order

I was following the existing convention for the probe momentum and parent particle momentum. I also didn't know a Direction class existed. Where is a reference for these classes?

I think that problem came from me misinterpreting the existing functionality. I wanted the Tool to load Genie events by either: taking the file path and event number from a WCSim file, taking the event number from a WCSim file but providing the corresponding Genie file yourself (this is needed for running on the grid), and loading only a Genie file with no WCSim input. I'll add back the functionality of matching the Genie event to the WCSim event based on the WCSim file name. One concern though, I wasn't aware of the existing naming nomenclature or file size. For the WCSim files that I have been generating, they are 1000 events per file instead of 500, and they are named wcsim_X.Y.root where X corresponds to gntp.X.ghep.root, and Y marks starting at the Y000 Genie event (so Y= 0 to 19 because there are 20000 events per Genie file). Should I change the manual matching functionality to reflect the every 1000 events? Or should I keep the 500, but regenerate the WCSim files I have been generating to match your nomenclature?

It compiled before in my dev copy of ToolAnalysis but I must have missed a line when I was transferring it over to a branch that would be ready for merger. Taking care of it and testing in the merger branch before I request another PR.

[jminock]

Just found the Position and Direction classes in DataModel. I'll switch over to Direction for all of the momentum variables.

I figure it's best to be backwards compatible so I'll regenerate my wcsim production again to match previous nomenclature and hold 500 events instead of 1000. I'll also return the manual matching code without adjustments so it fits that nomenclature and number of events.

Once again, I'll double check that this compiles before my next PR.

Closing this to make changes.

[marc1uk]

Yeah the Position and Direction class are just generic ANNIE classes similar to TVector3 but that you can save into a BoostStore as they implement the required serialize method. Other than that, i'm not sure what the "convention" is - what were you following? It's not like it matters, it's just what feels most intuitive really.

As for the number of events in the file, as always with hard-coded magic numbers this is a sign of a half-baked code (mine, in this case). I can't remember why the number of events per file is required - can't it be detected? But if there's a situation where it can't, then just make it a configuration variable, no need to regnerate any files. That would preserve backward compatibility, while making it more flexible too.

If you're just going to push another commit to this branch, there doesn't seem any need to close this and make a new PR; can we just keep this one open?

[jminock]

There were instances of probe momentum and parent particle momentum using Position instead of Direction so I followed just using Position. That's what I meant. I changed all instances of momentum to use Direction given it makes more sense. I had to add some functionality to the Direction class to do Unit vectors and compute Magnitude, which will be helpful for momentum analysis.

The hardcoded 500 was replaced with a config variable, and all checks passed!

[marc1uk]

ok, looks good. i get that someone, somewhere, has apparently done this to set this precedent, i was just curious who and where. but nevermind, looks good to merge, thanks again.