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.

Sign up for GitHub

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

Jump to bottom

Added SimDRCalorimeterHit for dual-readout #380

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Added SimDRCalorimeterHit for dual-readout #380

wants to merge 1 commit into from

Conversation

wonyongc
Copy link

BEGINRELEASENOTES

  • Added SimDRCalorimeterHit for optical photon dual-readout. Records number of scintillation/Cerenkov photons per hit as well as the wavelength/timing bins.

ENDRELEASENOTES

@wonyongc
Copy link
Author

Hi,
I have been developing a full simulation for a segmented crystal ECAL (CalVision/MAXICC collaborations) and am now submitting PRs to get the simulation into key4hep.
I needed to make a new datatype to store the scintllation/cerenkov hit information. So far I have simply extended the edm4hep schema with a custom 'edm4dr' schema in the simulation repo, but to merge into k4geo, it seems like it would be a better option (or the only one) to have this new schema merged into the main edm4hep.
Please let me know if I should take a different approach.
Thanks,
Wonyong

@wonyongc wonyongc mentioned this pull request Oct 25, 2024
Open
andresailer
andresailer reviewed Oct 30, 2024
View reviewed changes
edm4hep.yaml
Comment on lines +413 to +416
- int32_t eta // detector cell eta
- int32_t phi // detector cell phi
- int32_t depth // detector cell depth
- int32_t system // detector cell system
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you need these when there is already cellID?

andresailer
andresailer reviewed Oct 30, 2024
View reviewed changes
edm4hep.yaml
Comment on lines +419 to +422
- std::array<int32_t, 6000> nwavelen_cer // number of cerenkov wavelength hits
- std::array<int32_t, 6000> nwavelen_scint // number of scint wavelength hits
- std::array<int32_t, 6000> ntime_cer // number of cerenkov time hits
- std::array<int32_t, 6000> ntime_scint // number of scint hits
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is stored in these arrays? What is the index? Why 6000?
This feels awfully inefficient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andresailer andresailer andresailer left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@wonyongc @andresailer