-
Notifications
You must be signed in to change notification settings - Fork 21
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
Modified the LYSO target design #1498
base: trunk
Are you sure you want to change the base?
Conversation
Hi Sam, To help us understand how you know that your changes are good, please upload some plots in this discussion thread, explain what you ran to get them, and what they show. (This is the purpose of the second box, that you already checked when you made the PR. I unchecked it until you have shared some more material.) |
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.
Thank you for the images! Those look great :)
I just want to confirm that this runs and the geometry doesn't have any sneaky issues (e.g. overlaps that are not visible because they are small but could cause issues during the simulation). Can you share a configuration you've used to confirm that this geometry successfully runs a simulation?
We can then run the same configuration with
# sim is the simulator configuration object already created
sim.validate_detector = True
sim.verbosity = 5
so that ldmx-sw takes extra time to make sure the geometry is valid.
I have talked to Dr. James Oyang, @joyang8caltech, and he agreed to run some simulations using this geometry. Dr. Oyang, I forked a version of ldmx-sw into my github account and made the geometry modifications. It is at https://github.com/zwl0331/ldmx-sw You can clone it to run simulations with it. |
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.
Sorry it took me some time to look at this (DR writing...), I made some suggestions / questions in below.
Generarly I'd really like to see some tests that use this geometry, furthermore I'd suggest to either copy or rename the whole directory to r2, i.e. ldmx-lyso-r2-v14-8gev
the r
was for the revision, so it was exactly meant for this purposes. If you chose to rename, please add the info in the README that v4.1.4 is the last one to have ldmx-lyso-r1-v14-8gev
<!-- target box--> | ||
<constant name="target_dim_x" value="target_bar_dx" /> | ||
<constant name="target_dim_y" value="target_array2_dy" /> | ||
<constant name="target_thickness" value="2*target_bar_thickness + target_bar_gap" /> |
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.
Can you add a comment what this adds up to? Is it still 1.1 mm? If not what's the radiation length of the new thickness?
<!-- Target positions --> | ||
|
||
<!-- target array1--> | ||
<constant name="target_array1_first_bar_y" value="-(number_of_target_bars1 - 1) * (target_bar_dy + target_bar_gap) / 2" /> |
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 tried to identify what's array1 and array2 in the picture you attached, but I dont see it, can you please clarify?
@@ -83,7 +113,7 @@ | |||
<materialref ref="Vacuum"/> | |||
<solidref ref="target_area_box" /> | |||
|
|||
<loop for="x" to="number_of_bars" step="1"> | |||
<loop for="x" from="1" to="number_of_bars" step="1"> |
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.
this is not bad, but since you had
<variable name="x" value="1" />
this wont change anything. Anyway it's fine to change it
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 making this explicit is a good idea! navigating gdml files for variable definitions is usually not very fun.
@@ -100,16 +130,12 @@ | |||
</physvol> | |||
</loop> | |||
|
|||
<physvol copynumber="3"> | |||
<volumeref ref="target"/> | |||
<physvol name="lvTarget_phys"> |
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.
here too, I think when we add the sensitive volumes later on, that will look at the copy number
<auxiliary auxtype="Region" auxvalue="target" /> | ||
<!-- <auxiliary auxtype="VisAttributes" auxvalue="InvisibleShowDau"/> --> | ||
<auxiliary auxtype="DetElem" auxvalue="target"/> | ||
|
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.
again this aux info can be helpful, I'd suggest to keep them.
How is this going? Please see the specific geometry validation instructions posted here by @tomeichlersmith. I also wonder, @tomeichlersmith and @tvami, if we should have a validation workflow for geometry changes? (This is not the last time we'll make an update.) We do expect a whole lot of changes to distributions, so it's not clear what tests are meaningful, apart from that things run, and the overlap check tom mentioned. |
By the way @zwl0331, @joyang8caltech, the plastic geometry doesn't look right. We expect to have 24 bars in each plastics array, so they are equal height but staggered in y. |
We need to have thirty two 3mm height and 40mm long LYSO bars so that the active LYSO target will have the same coverage as tungsten target.
On Dec 16, 2024, at 6:17 AM, bryngemark ***@***.***> wrote:
By the way @zwl0331<https://github.com/zwl0331>, @joyang8caltech<https://github.com/joyang8caltech>, the plastic geometry doesn't look right. We expect to have 24 bars in each plastics array, so they are equal height but staggered in y.
—
Reply to this email directly, view it on GitHub<#1498 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/A24FEATKUB4RTRRVJDB5Q4T2F3OI5AVCNFSM6AAAAABSNDXYUKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNBVG42DOOJYGM>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
I am updating ldmx-sw, here are the details.
What are the issues that this addresses?
This resolves #1370 and implements the new design for the LYSO target. It consiste of two layers. The first layer has 32 bar, while the second has 33 bars.
Check List