-
Notifications
You must be signed in to change notification settings - Fork 19
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
Support for PandoraPFA on ALLEGRO #31
base: master
Are you sure you want to change the base?
Conversation
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 think we need to greatly reduce the code duplication here in particular in view of the migration of this to a Gaudi algorithm soon. Just copying everything to change a line here and there is not sensible
src/DDCaloHitCreatorALLEGRO.cc
Outdated
|
||
// check if the hit in barrel | ||
// FIXME! AD: ECal barrel systemID is hardcoded | ||
if (cellIdDecoder(pCaloHit)["system"] == 4) |
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.
This is the only change in this function, apart from removing some other block you don't care about?
Can you rather use DDCaloHitCreaters function and add something like
//...
ifUseSystemId && cellIdDecoder(pCaloHit)["system"] == ecalSystemID
//...
To the existing if statement?
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 as you have suggested
src/DDCaloHitCreatorALLEGRO.cc
Outdated
|
||
// check if the hit in barrel | ||
// FIXME! AD: HCal barrel systemID is hardcoded here | ||
if (cellIdDecoder(pCaloHit)["system"] == 8) |
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.
This is the only change in this function, apart from removing some other block you don't care about?
Can you rather use DDCaloHitCreaters function and add something like
//...
ifUseSystemId && cellIdDecoder(pCaloHit)["system"] == hcalSystemID
//...
To the existing if statement?
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 as you have suggested.
src/DDCaloHitCreatorALLEGRO.cc
Outdated
|
||
float absorberCorrection(1.); | ||
|
||
if (isInBarrelRegion) |
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 just set m_settings.m_coilOuterR
to 0 and use the existing function from DDCaloHitCreator? Or add a flag to disable the isWithinCoil
flag?
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 as you have suggested.
subDetectorTypeMap[pandora::COIL] = coilParameters; | ||
*/ | ||
|
||
// FIXME! AD: below an ugly way is used to define the geometry of the Solenoid since we do not have the LayeredCalorimeterData for Coil in ALLEGRO |
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 add this to the coil?
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 not sure if I understand your question.
Do you mean why we do not have CaloExtension for COIL?
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.
Yes.
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's because I could not find how the coil/solenoid is built in ALLEGRO.
I guess it is currently not there.
Of course, this code will change in the future and caloExtension will be added.
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.
Yes, there is currently no actual coil, it is accounted for in the material of the outer radius side of the cryostat
src/DDGeometryCreatorALLEGRO.cc
Outdated
|
||
//------------------------------------------------------------------------------------------------------------------------------------------ | ||
|
||
void DDGeometryCreatorALLEGRO::SetDefaultSubDetectorParameters(const dd4hep::rec::LayeredCalorimeterData &inputParameters, const std::string &subDetectorName, |
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.
This function did not change apart from whitespace? Just use the inherited one?
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.
Now inherited one is used.
/** | ||
* @brief DDTrackCreatorALLEGRO class | ||
*/ | ||
class DDTrackCreatorALLEGRO : public DDTrackCreatorBase |
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 inherit from DDTrackCreatorCLIC and reduce the amount of copy-pasta?
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 functions re-used in the DDTrackCreatorALLEGRO are supposed to change for ALLEGRO. Currently only few things are disabled, but implementation of the (truth) tracks in the ALLEGRO is not finished yet.
Co-authored-by: Andre Sailer <[email protected]>
BEGINRELEASENOTES
ENDRELEASENOTES