search

Mu2e / Offline

Offline software for the Mu2e experiment
Apache License 2.0
8 stars 79 forks source link

Add TrkStrawHitMC provenance and guard against empty StrawDigiMCs #1291

Closed edcallaghan closed 1 week ago

edcallaghan commented 2 weeks ago

This PR addresses fallout from PR #1245. In particular, that PR implemented functionality which can produce dummy StrawDigiMC objects which do not actually contain any useful information. Issue #1272 is a discussion place for structural replacements for that situation, but as it stands these dummy objects can be produced and must be accounted for in downstream analysis, which is partially addressed and partially enabled here. This PR enables the use of the SelectRecoMC module on events which contain StrawDigis from purely external origin.

Changes to existing data structures: -TrkStrawHitMC: a provenance data member is added, in sync with and acting to the same effect as the newly-introduced provenance data member of StrawDigiMC. This allows to guard against inspecting the underlying information of a TrkStrawHitMC associated with a StrawDigiMC for which no meaningful truth information exists. This is not only desireable for correct analysis, but structurally necessary to short-circuit references of invalid pointers.

Changes to existing functionality: -SelectRecoMC: Loops over StrawDigiMCs which may attempt to inspect the truth information contain therein now check the provenance of the StrawDigiMC and do not proceed with such inspection if the provenance is External. TrkStrawHitMC objects which are instantiated inherit their provenance from the preexisting StrawDigiMC. -TrkMCTools::findMCTrk: A loop over StrawDigiMCs now checks the provenance field before inspecting truth information, as above. This is a situation where it appears that there one stl-vector is being constructed in sync with another, but such synchronization is not actually assumed or required anywhere.

FNALbuild commented 2 weeks ago

Hi @edcallaghan, You have proposed changes to files in these packages:

which require these tests: build.

@Mu2e/fnalbuild-users, @Mu2e/write have access to CI actions on main.

:hourglass: The following tests have been triggered for 5ceb93b9f7499e029af0e4fac35aeaa851df11ca: build (Build queue has 4 jobs)

About FNALbuild. Code review on Mu2e/Offline.

FNALbuild commented 2 weeks ago

:umbrella: The build tests failed for 5ceb93b9f7499e029af0e4fac35aeaa851df11ca.

Test Result Details
test with :white_check_mark: Command did not list any other PRs to include
merge :white_check_mark: Merged 5ceb93b9f7499e029af0e4fac35aeaa851df11ca at 684bac4f22a1fa29f6bfd661300432e492dccbdd
build (prof) :white_check_mark: Log file. Build time: 04 min 11 sec
ceSimReco :x: Log file. Return Code 1.
g4test_03MT :white_check_mark: Log file.
transportOnly :white_check_mark: Log file.
POT :white_check_mark: Log file.
g4study :white_check_mark: Log file.
cosmicSimReco :white_check_mark: Log file.
cosmicOffSpill :white_check_mark: Log file.
ceSteps :white_check_mark: Log file.
ceDigi :white_check_mark: Log file.
muDauSteps :white_check_mark: Log file.
ceMix :white_check_mark: Log file.
rootOverlaps :white_check_mark: Log file.
g4surfaceCheck :white_check_mark: Log file.
FIXME, TODO :large_orange_diamond: TODO (1) FIXME (2) in 3 files
clang-tidy :large_orange_diamond: 0 errors 527 warnings
whitespace check :white_check_mark: no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 5ceb93b9f7499e029af0e4fac35aeaa851df11ca after being merged into the base branch at 684bac4f22a1fa29f6bfd661300432e492dccbdd.

For more information, please check the job page here. Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

kutschke commented 2 weeks ago

@edcallaghan Does this need to be tested with your PR in Production? or are the failures a different issue?

FNALbuild commented 2 weeks ago

:memo: The HEAD of main has changed to 825dc593c6dbdb4054a1b746f46b61a538a7075e. Tests are now out of date.

edcallaghan commented 2 weeks ago

The failure is unrelated to the PR in Production. This has something to do with the root dictionaries (I think) for a struct which has been extended in this PR. I didn't see any equivalent related in my local testing, so I'll have to look into it.

edcallaghan commented 2 weeks ago

@kutschke Sorry, I may have been using a stale page yesterday, which is why I seemed to have ignored your comments! I agree with you that the multiple provenance classes is starting to look like the wrong pattern (and we haven't even gotten beyond tracker yet), so I've consolidated into a single DigiProvenance class. There's an argument that this "sounds" wrong in the TrkStrawHitMC context, but the reality is that it is a one-to-one pairing. @brownd1978 chatted a bit about how this will look outside of the tracker, and I think a good option is that we recommend the DigiProvenance class be a common structure for this book-keeping, shared between subsystems.

edcallaghan commented 2 weeks ago

FYI, I snuck in one extra provenance check to make the standard Digitize production workflow compatible with mixed samples.

edcallaghan commented 2 weeks ago

...and also a higher-level but similar-purpose interface KalSeedMC::ContainsSimulation which is necessary to iron out downstream issues in TrkAna. I do not foresee adding any more functionality to this PR.

kutschke commented 2 weeks ago

... and I like that you added ContainsSimulation() - it should reduce pilot error.

kutschke commented 1 week ago

@FNALbuild run build test

FNALbuild commented 1 week ago

:hourglass: The following tests have been triggered for 4ead963bfd60a36e6a92c84ef499687f772eacd9: build (Build queue has 4 jobs)

edcallaghan commented 1 week ago

@kutschke To checkpoint the status of this, DigiProvenance is now implemented as an alias of an EnumToStringSparse<> specialization, in the usual pattern, and there are no issues using this for the "intended" workflow of mixing external data with current simulation. So, barring further comments, this is ready to merge.

But something we've learned here is that, because StrawDigiMC contains an art::Ptr<> into a SimParticleCollection, which is itself not considered in the current digi mixing implementation, trying to digi-mix preexisting simulation will not work as expected, because the information in the preloaded StrawDigiMCs is incomplete, as art::Ptr<SimParticle>s cannot be resolved. We'll need to decide what the preferable way to deal with this: either by allowing to mix the full extent of the MC information necessary for complete MC truth inspection, or by dropping support for the MC truth information when digi-mixing preexisting simulation.

kutschke commented 1 week ago

I think you are saying that you plan to deal with the outstanding issues in a new PR. Is that correct? Will old workflows work correctly in the mean time?

edcallaghan commented 1 week ago

Yes, and yes. No existing workflows are effected, but the projected workflow for mixing simulation onto simulation needs to be adjusted, which can be a separate PR.

kutschke commented 1 week ago

but the projected workflow for mixing simulation onto simulation needs to be adjusted, which can be a separate PR.

That works for me. Presuming the CI comes in cleanly I will approve and merge

FNALbuild commented 1 week ago

:sunny: The build tests passed at 4ead963bfd60a36e6a92c84ef499687f772eacd9.

Test Result Details
test with :white_check_mark: Command did not list any other PRs to include
merge :white_check_mark: Merged 4ead963bfd60a36e6a92c84ef499687f772eacd9 at 465477c32de5577161ac706295354f07ac88af11
build (prof) :white_check_mark: Log file. Build time: 04 min 10 sec
ceSimReco :white_check_mark: Log file.
g4test_03MT :white_check_mark: Log file.
transportOnly :white_check_mark: Log file.
POT :white_check_mark: Log file.
g4study :white_check_mark: Log file.
cosmicSimReco :white_check_mark: Log file.
cosmicOffSpill :white_check_mark: Log file.
ceSteps :white_check_mark: Log file.
ceDigi :white_check_mark: Log file.
muDauSteps :white_check_mark: Log file.
ceMix :white_check_mark: Log file.
rootOverlaps :white_check_mark: Log file.
g4surfaceCheck :white_check_mark: Log file.
FIXME, TODO :large_orange_diamond: TODO (0) FIXME (9) in 13 files
clang-tidy :large_orange_diamond: 4 errors 1256 warnings
whitespace check :white_check_mark: no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 4ead963bfd60a36e6a92c84ef499687f772eacd9 after being merged into the base branch at 465477c32de5577161ac706295354f07ac88af11.

For more information, please check the job page here. Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.