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.
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
Add TrkStrawHitMC provenance and guard against empty StrawDigiMCs #1291
Changes from 2 commits
72cfe6a
5ceb93b
28d7058
d29d8f3
394d607
b3e6fe0
115882f
c4f9c8f
44f2a5f
d163621
e847289
79c5896
4ead963
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 support a default value, perhaps 'unknown' to flag errors downstream
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 EnumToStringSparse class template requires that the detail class contain an enumerator named "unknown" and it default constructs it's objects to have a value of "unknown".
You will also add the implementation for the two functions declared in the detail class.
I suggest you moved the TrkStrawHitProvenance to be it's own .hh and .cc . Then use them in KalSeedMC .
If you prefer to keep them in KalSeedMC, then create KalSeedMC.cc to hold the missing implementation.
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.
Of course, making the initialization manifest does make the code more readable.
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.
... or add a comment that it defaults to unknown.
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.
A default constructor is now explicitly defined for the
TrkStrawHitMC
struct, which initializes the provenance tounknown
.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 default provenance has been updated to
Simulation
, so that existing simulation will be deserialized correctly.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.
@kutschke The common
DigiProvenance
now inhabits a standalone src/inc pair.Re default values: maybe I misunderstand your commentary, but the discussion here is about how to correctly initialize the provenance data member of
TrkStrawHitMC
when deserializing preexisting simulation in which the provenance field did not exist. Since the default value for the actual provenance enum will beunknown
, as you asy, this needs to be explicitly overridden in theTrkStrawHitMC
constructor to mark the preexisting simulation as having been produced as such.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 restructure logic to avoid 'continue'
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.
Done.