eic
Updated TOF geometry parameters and template for comparison with geometry DB Detector-20231031150001
Briefly, what does this PR introduce?
Updated TOF geometry parameters and template for comparison with geometry DB Detector-20231031150001
What kind of change does this PR introduce?
- [ ] Bug fix (issue #__)
- [ ] New feature (issue #__)
- [ ] Documentation update
- [x] Other: __
Please check if this PR fulfills the following:
- [ ] Tests for the changes have been added
- [ ] Documentation has been added / updated
- [x] Changes have been communicated to collaborators
Does this PR introduce breaking changes? What changes might users need to make to their code?
Does this PR change default behavior?
Please add a description and change the PR title to something more descriptive.
Do you also need to change compact/tracking/definitions_crsterlake.xml?
Is cysterlake.xml the new geometry? If so, yes.
On Dec 2, 2023, at 4:12 AM, Wouter Deconinck @.***> wrote:
Please add a description and change the PR title to something more descriptive.
Do you also need to change compact/tracking/definitions_crsterlake.xml?
— Reply to this email directly, view it on GitHub https://github.com/eic/epic/pull/605#issuecomment-1837157917, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHCEISTRSJJPNORRTVDIRYLYHMZLZAVCNFSM6AAAAABAD4GD6WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZXGE2TOOJRG4. You are receiving this because you were assigned.
@nschmidtALICE Please take a look at tof_endcap.xml. The only effective change is to reduce the outer radius from 67 to 60cm.
Is craterlake.xml the new geometry? If so, yes.
This still needs to be applied.
This fails check-tracking-geometry, https://github.com/eic/epic/actions/runs/7116897580/job/19376630835?pr=605, and can't get merged without fixing that. This would prevent running EICrecon.
This fails check-tracking-geometry, https://github.com/eic/epic/actions/runs/7116897580/job/19376630835?pr=605, and can't get merged without fixing that. This would prevent running EICrecon.
Volume misconfiguration error usually shows when the onion structure is not obeyed (barrel+endcape moving from inside to out without overlap). Now checking in detail.
@yezhenyu2003 It's probably best to do the debugging steps locally or put more descriptive commit messages on the debugging steps. They are not currently telling me much. You can build epic under eicshell and then use the root -q -b scripts/test_ACTS.cxx+'("epic_craterlake.xml")' to check for overlaps in tracking geometry without running the whole pipeline.
@yezhenyu2003@wdconinc I cherrypicked the commit 373f59c because that was the last one before the "debugging" commits. The issue seems to be as follows
- The barrel tof and endcap tof are created as separate subassemblies https://github.com/eic/epic/blob/main/compact/tracking/definitions_craterlake.xml#L163-175
- Because the endcap tof assembly comes first and it only has endcap layers, a gap layer is created between them using the radial bounds of the endcaps. From the tracking geometry checker output, we can see the positive endcap is created with the following dimensions
01:22:38 D2A_CVH VERBOSE Create cylindrical TrackingVolume 'EndcapTOFSubAssembly::PositiveEndcap'.
01:22:38 D2A_CVH VERBOSE -> with given dimensions of (rMin/rMax/zMin/Max) = 34 / 601.727 / 1618.22 / 1936
This results in a gap layer creation which overlaps with the tof barrel
01:22:38 D2A_V:Barrel VERBOSE Configuration after container synchronisation
New container built with configuration: 599, 631 / -1446, 1986
- c: Barrel, current configuration: 599, 631 / -1446, 1986
Existing volume with configuration: 34, 601.727 / -1208.22, 1936 (Gap layer)
Probable solution: Trial and error with the envelope boundaries. Lifting the minR for the tof barrel slightly so that it goes above 601.727 might solve the issue. Alternatively, lower the maxR for endcap tof. Example: https://github.com/eic/epic/blob/8d45ca3affa3d849bf6e43f27320438bf782ddd6/src/BarrelPlanarMPGDTracker_geo.cpp#L303-306
@yezhenyu2003 Is this ready now? The pipeline seems to pass.
not yet but move to the right direction. Still want to play with some of the dimensions
@rahmans1 @wdconinc ready for merging @yezhenyu2003 Looking at the menagerie and see a notable difference. In the menagerie, the forward tof maxR is shown as 60 but i see that you have it at 57.5. Do you know if the values mentioned in the menagerie show a hypothetical envelope region or actual physical extent of the detector? If it's the later, are you cutting out coverage to avoid simulation overlap? https://eic.jlab.org/Geometry/Detector/Detector-20231031150001.html
They are envelopes. Sent from my iPhoneOn Dec 8, 2023, at 4:01 PM, Sakib Rahman @.***> wrote:
@rahmans1 @wdconinc ready for merging @yezhenyu2003 Looking at the menagerie and see a notable difference. In the menagerie, the forward tof maxR is shown as 60 but i see that you have it at 57.5. Do you know if the values mentioned in the menagerie show a hypothetical envelope region or actual physical extent of the detector? If it's the later, are you cutting out coverage to avoid simulation overlap? https://eic.jlab.org/Geometry/Detector/Detector-20231031150001.html
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>
They are envelopes. Sent from my iPhoneOn Dec 8, 2023, at 4:01 PM, Sakib Rahman @.> wrote: @rahmans1 @wdconinc ready for merging @yezhenyu2003 Looking at the menagerie and see a notable difference. In the menagerie, the forward tof maxR is shown as 60 but i see that you have it at 57.5. Do you know if the values mentioned in the menagerie show a hypothetical envelope region or actual physical extent of the detector? If it's the later, are you cutting out coverage to avoid simulation overlap? https://eic.jlab.org/Geometry/Detector/Detector-20231031150001.html —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.>
Ok. If the menagerie is showing envelopes, should it be updated to match what you currently have? It is a little bit confusing for someone to compare without context. Also, how would one keep track of the physical extents of the actual detector?
The geometry db is the envelope as far as I understand it. I would not change that Sent from my iPhoneOn Dec 8, 2023, at 4:10 PM, Sakib Rahman @.***> wrote:
They are envelopes. Sent from my iPhoneOn Dec 8, 2023, at 4:01 PM, Sakib Rahman @.> wrote: @rahmans1 @wdconinc ready for merging @yezhenyu2003 Looking at the menagerie and see a notable difference. In the menagerie, the forward tof maxR is shown as 60 but i see that you have it at 57.5. Do you know if the values mentioned in the menagerie show a hypothetical envelope region or actual physical extent of the detector? If it's the later, are you cutting out coverage to avoid simulation overlap? https://eic.jlab.org/Geometry/Detector/Detector-20231031150001.html —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.>
Ok. If the menagerie is showing envelopes, should it be updated to match what you currently have? It is a little bit confusing for someone to compare without context. Also, how would one keep track of the physical extents of the actual detector?
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>
If you or some expert is willing to help, please go for it. Best,ZhenyuSent from my iPhoneOn Dec 8, 2023, at 9:58 PM, Wouter Deconinck @.***> wrote: @wdconinc commented on this pull request.
I am having trouble approving this. We want to make sure the detectors are in the right place, which could be inside the envelopes specified by the detector parameter table, but we also want to make sure the detector parameter table numbers are there as the envelopes in the simulation. Right now there appear different numbers here and we can't just tell the project "yes, we now agree" much less make the same statement easily after they change the detector placement. It is possible that you cannot place the detectors with separate barrel and endcap subassemblies for acts given the current dimensions, but fudging the numbers until it works is not the right solution then. Instead we should then create a barrel+endcap subassembly for acts that has the right dimensions of the envelopes and the detectors inside them.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>
@yezhenyu2003 I understand this is frustrating, but it is not our responsibility to implement the geometry for every subsystem. That is the responsibility of the detector subsystem experts. We can help but, no, we won't "go for it". The relevant part of the requirements on the geometry for compatibility with Acts is at https://acts.readthedocs.io/en/latest/plugins/dd4hep.html#prerequisites.
Understood. From TOF of view, the geometries are good for now. ZhenyuSent from my iPhoneOn Dec 9, 2023, at 12:49 AM, Wouter Deconinck @.***> wrote: @yezhenyu2003 I understand this is frustrating, but it is not our responsibility to implement the geometry for every subsystem. That is the responsibility of the detector subsystem experts. We can help but, no, we won't "go for it". The relevant part of the requirements on the geometry for compatibility with Acts is at https://acts.readthedocs.io/en/latest/plugins/dd4hep.html#prerequisites.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>
Need to resolve ACTS issues before merging
What acts issues are you referring to?
@yezhenyu2003 We are tagging a new release in preparation for February campaign soon. I see there are are some rebase conflicts that needs to be resolved. Is this PR otherwise ready or there is something else missing? @Chao1009 Could you clarify your comment?
Yes from my point of view, the PR is ready
On Feb 6, 2024, at 8:09 PM, Sakib Rahman @.***> wrote:
@yezhenyu2003 https://github.com/yezhenyu2003 We are tagging a new release in preparation for February campaign soon. I see there are are some rebase conflicts that needs to be resolved. Is this PR otherwise ready or there is something else missing?
— Reply to this email directly, view it on GitHub https://github.com/eic/epic/pull/605#issuecomment-1930586407, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHCEISVIKPUYYJYOETRBKUTYSJ5XDAVCNFSM6AAAAABAD4GD6WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZQGU4DMNBQG4. You are receiving this because you were mentioned.
@yezhenyu2003 sorry I missed the comments. Could you please verify if the changes are up-to-date? I have resolved the conflicts with the main branch and squashed all commits into one.
@ Chao1009 these are not up-to-date anymore.
I am copying Kentaro and Tommy who are leading the TOF simulation efforts. I think they can provide the latest TOF geomtries
Zhenyu
On Oct 2, 2024, at 8:20 AM, Chao Peng @.***> wrote:
@yezhenyu2003 https://github.com/yezhenyu2003 sorry I missed the comments. Could you please verify if the changes are up-to-date? I have resolved the conflicts with the main branch and squashed all commits into one.
— Reply to this email directly, view it on GitHub https://github.com/eic/epic/pull/605#issuecomment-2388956689, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHCEISSIW4QWXLR4I6IPJO3ZZQFNPAVCNFSM6AAAAABPH26YVKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOBYHE2TMNRYHE. You are receiving this because you were mentioned.
@yezhenyu2003 Sorry I was out of the loop in this discussion so I may not understand your question well, but I will provide as much details as I could regarding TOF geometry.
We had an accepted pull request about BTOF previously because each stave used to be a long continuous sensitive area. We modified it to break the area into small individual sensors and introduce dead spaces.
The most updated information we have shows that BTOF sensors are double sided (i.e. sensors on both side of the stave) and updated ETOF geometry is also different from what epic currently has. We plan on submitting another pull request to update the geometry sometime this week or next week.
I hope this answers your question. Please let me know if you want anymore information.
@yezhenyu2003 Thank you for clarifying it. Since the update is not valid anymore, I will close this PR. A new PR should be opened if any of the TOF geometries need to be updated,