-
Notifications
You must be signed in to change notification settings - Fork 13
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
[MUONDigi] Adding a digitizer of IDEA Muon system. #24
base: master
Are you sure you want to change the base?
Conversation
Add missing GaudiAlgLib
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.
Very nice Mahmoud! Here are already a few questions/comments
|
||
datatypes: | ||
|
||
extension::MCRecoMuonSystemDigiAssociation: |
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.
Why not using MCRecoTrackerAssociation
https://github.com/key4hep/EDM4hep/blob/main/edm4hep.yaml#L634 ?
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.
Can it be used for linking between digi and sim? I fount it relates between reco and sim..
On another hand I'd like to see if there is an association also to store the validation data (e.g: simDigipPositionDifference)
// Detector efficiency | ||
FloatProperty m_efficiency{this, "efficiency", 0.95, "Detector efficiency"}; | ||
|
||
// Declaration of validation distribution |
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.
Can you add a flag, set to false by default, to produce these collections? See e.g. https://github.com/key4hep/k4RecTracker/blob/master/DCHdigi/include/DCHsimpleDigitizerExtendedEdm.h#L78 (the branch will unfortunately still be there I am afraid)
DataHandle<podio::UserDataCollection<double>> m_simDigiDifferenceZ{"simDigiDifferenceZ", Gaudi::DataHandle::Writer, this}; // mm | ||
|
||
// Random Number Service | ||
IRndmGenSvc* m_randSvc; |
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.
@jmcarcell let us know if a consensus was reached on which random number generator we should use
// y resolution in mm | ||
FloatProperty m_y_resolution{this, "yResolution", 1.0, "Spatial resolution in the y direction [mm]"}; | ||
|
||
// z resolution in mm |
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 don't understand why we need 3 numbers here. Since you do the smearing in local coordinates (as it should), 2 numbers should be sufficient no? OR at least we should put ine by default to 0, otherwise we really smear in the 3 dimensions.
BEGINRELEASENOTES
Defining a first draft for the muon system digitizer, which can do the following tasks:
ENDRELEASENOTES