-
Notifications
You must be signed in to change notification settings - Fork 50
Flat ob #933
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
base: main
Are you sure you want to change the base?
Flat ob #933
Conversation
for more information, see https://pre-commit.ci
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.
Thanks for the work, Sam!
Please pre-define variables as you can, instead of through in magic numbers in the detector block. I tagged a few occurrences I noticed, but please go over the entire file.
Plus, we had the stave tilt angle to create small overlaps in acceptance on purpose. For this flat stave geometry, I recommend we either keep the angle, or place the staves at two radii as the actual design did.
<constant name="SiBarrelMod_angle" value="SiBarrel_angle"/> | ||
<constant name="SiBarrelMod_dz" value="SiBarrel_dz"/> | ||
Edited by Aditya Yalavatti and Sam Henry, University of Oxford | ||
</comment> |
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.
Please add a reference here, a design version, or drawings/ presentation would be nice.
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.
Thanks Shujie, I have now added pre-defined variables for all the numbers that we introduced. I added a link to the meeting last week, where I added this presentation as an update: https://indico.bnl.gov/event/29542/contributions/112738/attachments/64431/110856/flat_OB%20update%205%20September%202025.pdf
The staves are now placed at two radii 6mm apart to match the design, so the tilt is now set to 0.
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.
Thanks! Now I see the PR failed several overlap checks. Can you fix 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.
I think that's fixed now - it was a transcription error when I put in the predefined variables
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.
Github is still saying there is an overlap, but when I run checkOverlaps and python scripts/checkOverlaps on my system, it doesn't flag anything. Is github out of sync with my update? Is it checking something else? I thought I had left the layer envelope volume unchanged so there wouldn't be any overlap issues with anything outside.
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.
Did you use the tolerance flag, e.g. checkOverlaps -t 0.01 -c ${DETECTOR_PATH}/${DETECTOR}.xml ?
@wdconinc do you think this geometry error msg https://github.com/eic/epic/actions/runs/17547963093/job/49834060533?pr=933 is related to the known ACTS issue?
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 is likely the known ACTS issue. There are four (one is doubly reported) known overlaps reported in current main, at substantially similar (but slightly different) positions as what's shown here:
3:2320:10:46:12 D2A_LAC ERROR Layers are overlapping at: -2210.61. This should never happen. Please check your geometry description.
4:2352:10:46:12 D2A_LAC ERROR Layers are overlapping at: 0.385. This should never happen. Please check your geometry description.
5:2971:10:46:27 D2A_LAC ERROR Layers are overlapping at: 1.25625. This should never happen. Please check your geometry description.
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.
@ShujieL Yes, checkOverlaps -t 0.01 -c ${DETECTOR_PATH}/${DETECTOR}.xml gave "Number of illegal overlaps/extrusions : 0"
python scripts/checkOverlaps.py -t 0.01 -c ${DETECTOR_PATH}/${DETECTOR}.xml listed a lot of things, but not for the sagitta or outer barrels
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.
@ShujieL I've made the changes we discussed yesterday:
The barrels are now at the correct radii and the stave width is 38mm for both barrels (as in the current design)
The barrels have been shortened so they don't overlap with the inner support cone
The layer envelope volume has been adjusted accordingly
This means the geometry is a better match to the true design at small eta. Once the inner support geometry is updated, it will be straightforward to put the barrel lengths to the correct values
compact/tracking/silicon_barrel.xml
Outdated
<rphi_layout phi_tilt="SiBarrelStaveTilt_angle" nphi="SiBarrelStave1_count" phi0="0.0" rc="SiBarrelMod1_rmin" dr="0.0 * mm"/> | ||
<z_layout dr="0.0 * mm" z0="0.0 * mm" nz="1"/> | ||
<rphi_layout phi_tilt="0.0" nphi="46" phi0="0.0" | ||
rc="SiBarrelMod1_rmin+5.5*mm" dr="6.0 * 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.
Please define the 5.5*mm, and dr at the top of the file. Also, I suspect nphi=46 should be SiBarrelStave1_count.
compact/tracking/silicon_barrel.xml
Outdated
|
||
<!-- Carbon fibre braces --> | ||
<module_component name="Brace0" material="CarbonFiber" | ||
width="SiBarrelStave2_width*0.9" |
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.
Please also define 0.9 here and in a few other places.
for more information, see https://pre-commit.ci
flat_OB update 5 September 2025.pdf |
for more information, see https://pre-commit.ci
Briefly, what does this PR introduce?
This is a new version of the outer barrel geometry silicon_barrel.xml to better match the CAD files. Changes include;
Replacing the carbon fiber frame with all flat components
Adding carbon fiber braces to model peaks in the material thickness due to module structure
Adding gap in the active silicon in the centre of modules to model the dead area
Replacing the tilted stave alignment with a castellated arrangement
Small changes to widths and thicknesses
This version matches the material thickness of CAD model to ~5% and is significantly less than the current epic-main geometry
Work done by Aditya Yalavatti and Sam Henry, University of Oxford
What kind of change does this PR introduce?
Please check if this PR fulfills the following:
Does this PR introduce breaking changes? What changes might users need to make to their code?
Does this PR change default behavior?