-
Notifications
You must be signed in to change notification settings - Fork 60
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
Monolithic fiber DRC memory usage reduction update, using IDEA o1 v03 #346
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.
The first round of comments (note that assembly box should also be changed - which I know that is ongoing)
@BrieucF PR looks good to me. Would you mind reviewing it? Otherwise, I think we can merge this for the sake of material budget studies. |
Thanks @SanghyunKo and @swkim95 ! Alvaro will do the review as soon as possible. I'd just ask for now to add a description of the detector builder here |
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 can only give rather superficial comments, because I cannot understand the details of the geometry and implementation.
I would say that all of this needs a lot more comments in the code to understand the intent and for maintainability.
I probably missed some unit less numbers as well.
I see some warnings when compiling this version. Can you try to fix them?
|
And actually, the code does not compile: https://github.com/key4hep/k4geo/actions/runs/9784873250/job/27016711979?pr=346#step:3:440 |
Final remarks before merging the PR
|
Thanks Sanghyun. For another PR: given the currently large time taken per event, we will disable this detector by default in IDEA_o1_v03, making sure that it remains tested with CTests. But we will take care of that. |
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.
Approving this now because
- having the geometry already unlocks some studies on multiple scattering
- having it in the central repository makes sure it will be tested from now on and makes life easier for people needing the detector
- the sensitive action will be improved in subsequent PR
Can we merge this? |
There are conflicts... |
With what? |
…ardcoded geometry and add dimensions in xml file
…e, Clarify the units, write detector description
…constructor to remove warnings
ab84a09
to
fce0f31
Compare
BEGINRELEASENOTES
detector/calorimeter/dual-readout
directoryFCCee/IDEA/compact/IDEA_o1_v03
directoryplugin
directoryENDRELEASENOTES
Purpose
Tests