Skip to content
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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Archil-AD
Copy link

BEGINRELEASENOTES

  • The "DetectorName" parameter is added to the DDPandoraPFANewProcessor which allows to setup the DDGeometryCreator, DDCaloHitCreator and DDTrackCreator for the ALLEGRO detector;
  • Three new classes are created that are specific for the ALLEGRO detector: DDGeometryCreatorALLEGRO, DDCaloHitCreatorALLEGRO and DDTrackCreatorALLEGRO.

ENDRELEASENOTES

Copy link
Contributor

@andresailer andresailer left a 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 Show resolved Hide resolved

// check if the hit in barrel
// FIXME! AD: ECal barrel systemID is hardcoded
if (cellIdDecoder(pCaloHit)["system"] == 4)
Copy link
Contributor

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?

Copy link
Author

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


// check if the hit in barrel
// FIXME! AD: HCal barrel systemID is hardcoded here
if (cellIdDecoder(pCaloHit)["system"] == 8)
Copy link
Contributor

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?

Copy link
Author

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.


float absorberCorrection(1.);

if (isInBarrelRegion)
Copy link
Contributor

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?

Copy link
Author

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
Copy link
Contributor

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?

Copy link
Author

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Copy link
Author

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.

Copy link

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


//------------------------------------------------------------------------------------------------------------------------------------------

void DDGeometryCreatorALLEGRO::SetDefaultSubDetectorParameters(const dd4hep::rec::LayeredCalorimeterData &inputParameters, const std::string &subDetectorName,
Copy link
Contributor

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?

Copy link
Author

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
Copy link
Contributor

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?

Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants