-
Notifications
You must be signed in to change notification settings - Fork 82
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add TrkStrawHitMC provenance and guard against empty StrawDigiMCs #1291
Merged
kutschke
merged 13 commits into
Mu2e:main
from
edcallaghan:update_TrkStrawHitMC_provenance
Jul 1, 2024
Merged
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
72cfe6a
propagate tracker mc provenance from StrawDigiMC -> TrkStrawHitMC
edcallaghan 5ceb93b
improved commentary
edcallaghan 28d7058
invert StrawDigiProvenance checks to avoid continues
edcallaghan d29d8f3
reduce footprint of StrawDigiProvenance -> TrkStrawHitProvenance cast…
edcallaghan 394d607
several TrkStrawHitMC-relevant changes:
edcallaghan b3e6fe0
consolidate StrawDigi-/TrkStrawHit- provenance classes into single Di…
edcallaghan 115882f
make DigiProvenance inherit from its EnumToStringSparse<> specializat…
edcallaghan c4f9c8f
exchange bracketed project includes for quoted
edcallaghan 44f2a5f
add intermediate StringedDigiProvenance class to root dictionary
edcallaghan d163621
guard against empty mc truth in StrawDigiMCFilter
edcallaghan e847289
const-qualify DigiProvenance::ContainsSimulation
edcallaghan 79c5896
propagate logical-or of constitutent DigiProvenances to KalSeedMC pro…
edcallaghan 4ead963
factor DigiProvenance::ContainsSimulation -> free containsSimulation,…
edcallaghan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
// Flag to designate a digi as having been produced from simulation, | ||
// read from preexisting data, or a hybridization fo the two | ||
// Ed Callaghan, 2024 | ||
|
||
#ifndef MCDataProducts_DigiProvenance_hh | ||
#define MCDataProducts_DigiProvenance_hh | ||
|
||
// stl | ||
#include <map> | ||
#include <string> | ||
|
||
// mu2e | ||
#include "Offline/GeneralUtilities/inc/EnumToStringSparse.hh" | ||
|
||
namespace mu2e{ | ||
// enum equipped with std::string descriptions | ||
class DigiProvenanceDetail{ | ||
public: | ||
enum enum_type {unknown=0, Simulation, Mixed, External}; | ||
static std::string const& typeName(); | ||
static std::map<enum_type, std::string> const& names(); | ||
}; | ||
using StringedDigiProvenance = EnumToStringSparse<DigiProvenanceDetail>; | ||
|
||
class DigiProvenance: public StringedDigiProvenance{ | ||
public: | ||
DigiProvenance(StringedDigiProvenance); | ||
bool ContainsSimulation() const; | ||
}; | ||
} | ||
|
||
#endif |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Three things. First, EnumToStringSparse does not have a virtual d'tor so we should not inherit from it. That's easy to fix, we can give it a virtual d'tor and also add the other 4 rule of 5 functions as =default. ( My style is if the compiler will do the right thing for all rule of 5 functions I just leave them out and add a one line comment to that effect - I see that EnumToStringSparse is so old that the rule of 5 was then the rule of 3.) Go ahead and edit EnumToStringSparse.
The second issue is the CSAID C++ experts strongly advise against using inheritance when containment will do. We do violate this in some places. I think we are OK here.
Third, I think that if you add
typedef StringedDigiProvenance::enum_type enum_type;
(or try the equivalent using would be better ).
Then downstream code can avoid needing the DigiProvenanceDetail:: and you may also be able to avoid the static casts. I am not 100% sure that I have all details right here but I am pretty sure I am close.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After sleeping on it, there is an different solution that avoids introducing the third type and avoids issue with inheritance, just use a free function:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The free-function approach appears to be the path of least resistance --- it is preferable, IMO, to the extra baggage needed to make another distinct class pass muster. (FWIW, I went with inheritance over containment here because otherwise there are extra implementations to add to the list (i.e. forwarding all of the member functions of
EnumToStringSparse<>
)).That being said, I'm seeing issues with serialization of that simpler implementation (
using DigiProvenance = EnumToStringSparse<DigiProvenanceDetail>
) manifesting as data corruption after deserializing, in which the actualenum_type
not being set correctly. Since this should be simpler the scenario, I'm not really sure what's going wrong. I'll have another try at figuring it out, but if I can't then I may need to just push updates toEnumToStringSparse<>
to make it "inheritable."There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the slow response.
Can you point me to your working code that is failing and tell me what the error message is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries. I have the changes in a branch at https://github.com/edcallaghan/mu2e-Offline/tree/update_TrkStrawHitMC_provenance_free_functions. If you compile that (should work at head) and run the contents of
~ejc3/public/DigiProvenance
, you can reproduce for yourself. (This is a pair of jobs exemplified in.../run.sh
, in which we firstphysics-mix'' [pm] a few NoPrimary [np] events, and then
digi-mix'' [dm] a few CEs into them [ce]). You can see the error in.../dm-ce.log
, the relevant bit pasted here:But: my statement that the problem is in the serialization of the provenance was confused and, I think, incorrect. After another round of less-sloppy testing, the problem presents with the current version in this PR as well.
I suspect that the actual problem is with the construction of a new
StrawDigiMC
at https://github.com/Mu2e/Offline/blob/main/TrackerMC/src/StrawDigiBundleCollection.cc#L223, ultimately stemming from the same "scope constraint" on use of art data products as I mention in #1290. That has nothing to do with the implementation ofDigiProvenance
, so I can push the simpler implementation for that here. If I'm right, then another PR will need to be opened which addresses the invalidPtr
s.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am looking at it now.
Just a wild guess and it's from memory. If your are making two data products, ACollection and BCollection, both in the same module, and if one contains Ptrs into the other, you cannot dereference the Ptrs until the module has returned. So they only work in downstream modules. The reason is that the pointee data product does not exist until after the return ( the move and the return are like a database two phase commit ).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make dm-ce.fcl readable by me.
ls -l ~ejc3/public/DigiProvenance/dm-ce.fcl
-rw-------+ 1 ejc3 nobody 4177 Jun 30 15:46 /nashome/e/ejc3/public/DigiProvenance/dm-ce.fcl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like my wild guess is irrelevant The changes to Filters/src/StrawDigiMCFilter_module.cc look simple enough. So I agree it's likely that the bug is earlier in the workflow.
That's for making the change to the free function and fixing the typo. My reason for suggesting the free function was the same as in your reply.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry,
~ejc3/public/DigiProvenance/dm-ce.fcl
is now public-readable.But, that being said, I think the issue here is actually much dumber than what I was getting at. When "digi mixing," I am mixing in the
StrawDigiMC
s, each of which contains anart::Ptr<SimParticle>
into aSimParticleCollection
. But I don't actually mix theSimParticleCollection
in, so of course the pointer can't be dereferenced.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are all of the fixes in now?