Origin/calo geom g4 update #1311
Conversation
|
Hi @bechenard,
which require these tests: build. @Mu2e/write, @Mu2e/fnalbuild-users have access to CI actions on main. The following users requested to be notified about changes to these packages: ⌛ The following tests have been triggered for d41d099: build (Build queue has 1 jobs) |
|
☔ The build tests failed for d41d099.
N.B. These results were obtained from a build of this Pull Request at d41d099 after being merged into the base branch at af80f04. For more information, please check the job page here. |
|
@bechenard CalorimeterMother was originally defined as the volume that George allocated to the calorimeter. It was a way to enforce that the geometry was compliant with space allocations. Have things now changed so that some volumes (cable runs?) need to cross that boundary? If so, that's Ok but I want to make sure that the we are not getting rid of CalorimeterMother without a good reason. |
|
HI Rob,
The full CalorimeterMother volume interfered with the tracker cable run,
and this made the implementation unnecessary complicated. Given the
"weird" calo geometry, I split the mother volume into a disk mother
volume, and a FEB mother volume. Both volume have the same z length as
CalorimeterMother and the radial extent of the FEB mother volume matches
that of the calorimeterMother. If you wish, I can group the the disk
mother and FEB mother into a new calorimeter mother (as a combination of
two volumes).
Removing CalorimeterMother also crated some issues with the scripts, so
adding one back would simplify tests. I;m also addressing the clang tidy
warnings.
Let me know if there is anything else I should address at the conceptual
level.
Cheers,
B
On 7/16/2024 1:38 PM, Rob Kutschke wrote:
@bechenard <https://github.com/bechenard> CalorimeterMother was
originally defined as the volume that George allocated to the
calorimeter. It was a way to enforce that the geometry was compliant
with space allocations. Have things now changed so that some volumes
(cable runs?) need to cross that boundary? If so, that's Ok but I want
to make sure that the we are not getting rid of CalorimeterMother
without a good reason.
—
Reply to this email directly, view it on GitHub
<#1311 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABJ27JDLF5TOUN76LQX3ZZDZMWAGBAVCNFSM6AAAAABK7IRG4OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMZRG44DONRXGY>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--------------eQSbmgNuM9Mee5dUPtmisFK0
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 8bit
<!DOCTYPE html><html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
<p>HI Rob,</p>
<p>The full CalorimeterMother volume interfered with the tracker
cable run, and this made the implementation unnecessary
complicated. Given the "weird" calo geometry, I split the mother
volume into a disk mother volume, and a FEB mother volume. Both
volume have the same z length as CalorimeterMother and the radial
extent of the FEB mother volume matches that of the
calorimeterMother. If you wish, I can group the the disk mother
and FEB mother into a new calorimeter mother (as a combination of
two volumes).</p>
<p>Removing CalorimeterMother also crated some issues with the
scripts, so adding one back would simplify tests. I;m also
addressing the clang tidy warnings.</p>
<p>Let me know if there is anything else I should address at the
conceptual level.</p>
<p>Cheers,</p>
<p>B<br>
</p>
<div class="moz-cite-prefix">On 7/16/2024 1:38 PM, Rob Kutschke
wrote:<br>
</div>
<blockquote type="cite" ***@***.***">
<p dir="auto"><a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/users/bechenard/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://github.com/bechenard" ***@***.***</a>
CalorimeterMother was originally defined as the volume that
George allocated to the calorimeter. It was a way to enforce
that the geometry was compliant with space allocations. Have
things now changed so that some volumes (cable runs?) need to
cross that boundary? If so, that's Ok but I want to make sure
that the we are not getting rid of CalorimeterMother without a
good reason.</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br>
Reply to this email directly, <a href="#1311 (comment)" moz-do-not-send="true">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/ABJ27JDLF5TOUN76LQX3ZZDZMWAGBAVCNFSM6AAAAABK7IRG4OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMZRG44DONRXGY" moz-do-not-send="true">unsubscribe</a>.<br>
You are receiving this because you were mentioned.<img src="https://github.com/notifications/beacon/ABJ27JCY6RNGKCWRQFUUIT3ZMWAGBA5CNFSM6AAAAABK7IRG4OWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTUFAZQJY.gif" alt="" moz-do-not-send="true" width="1" height="1"><span style="color: transparent; font-size: 0; display: none; visibility: hidden; overflow: hidden; opacity: 0; width: 0; height: 0; max-width: 0; max-height: 0; mso-hide: all">Message
ID: <span><Mu2e/Offline/pull/1311/c2231787676</span><span>@</span><span>github</span><span>.</span><span>com></span></span></p>
<script type="application/ld+json">[
{
***@***.***": "http://schema.org",
***@***.***": "EmailMessage",
"potentialAction": {
***@***.***": "ViewAction",
"target": "#1311 (comment)",
"url": "#1311 (comment)",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
***@***.***": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>
</blockquote>
</body>
</html>
…--------------eQSbmgNuM9Mee5dUPtmisFK0--
|
The CalorimeterMother is also used to set UserLimits and some of the stepping cuts. It's referenced in some of the fcl in Production. At one time I think it was used as a stopping volume in the cosmic workflows but I think that's no longer true - check with Yuri and Ralf. Your reason for removing it (volumes that need to cross it's boundaries ) is OK with me so long as you can manage all of the downstream effects. About the clang tidy warnings - deal with the ones from your own code. Most will come from included headers - many of which I wrote. |
|
📝 The HEAD of |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for 8afaff6: build (Build queue has 1 jobs) |
|
☔ The build tests failed for 8afaff6.
N.B. These results were obtained from a build of this Pull Request at 8afaff6 after being merged into the base branch at a597848. For more information, please check the job page here. |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for 2f968ed: build (Build queue has 1 jobs) |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for d182db8: build (Build queue is empty) |
|
☀️ The build tests passed at d182db8.
N.B. These results were obtained from a build of this Pull Request at d182db8 after being merged into the base branch at 2540af1. For more information, please check the job page here. |
|
@kutschke All good, ready to merge. It turns out it was more confusing to get all signed/unsigned rather than using to const_cast, so I kept all integers and added the const_cast. I need a few more GOTOs, but this is for another PR! |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for d182db8: build (Build queue is empty) |
FNALbuild
added
build pending
build running
and removed
build finished
build pending
labels
|
☀️ The build tests passed at d182db8.
N.B. These results were obtained from a build of this Pull Request at d182db8 after being merged into the base branch at e8fecef. For more information, please check the job page here. |
|
I did valCompare on reco_00.fcl + _01.fcl and on ceSimReco_00.fcl + _01.fcl . The results are at: https://home.fnal.gov/~kutschke/Mu2e/PR1311/reco/results.html This is 2x the stats as before. Both look Ok to me. The reco input files are 4 files from dig.mu2e.CeEndpointMix1BBSignal.MDC2020r_best_v1_0 . There are bigger changes in ceSimReco than in reco, presumably because of random number changes. Bertrand: please let us know whether or not the comparisons pass your inspection. |
|
I'm guessing the change in timing:
https://home.fnal.gov/~kutschke/Mu2e/PR1311/reco/Validation_KalSeeds_KKDeM_noName_CCDt.gif
comes from changing the calorimeter crystal positions. So this change is
NOT compatible with MDC2020 datasets. We will discuss on Wednesday the
current plan for branching incompatible changes, I ask that merging be
delayed until after that.
…On Mon, Jul 29, 2024 at 9:01 AM Rob Kutschke ***@***.***> wrote:
***@***.**** approved this pull request.
—
Reply to this email directly, view it on GitHub
<#1311 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABAH572R3NRMPVINE4537ALZOZRMHAVCNFSM6AAAAABK7IRG4OVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEMBVGM4DKNBXHE>
.
You are receiving this because you are on a team that was mentioned.Message
ID: ***@***.***>
--
David Brown ***@***.***
Office Phone (510) 486-7261
Lawrence Berkeley National Lab
M/S 50R5008 (50-6026C) Berkeley, CA 94720
|
That's my plan. |
|
📝 The HEAD of |
Updated description of the calorimeter with latest geometry drawings. I changed the way the cable runs were coded to insulate the calorimeter from the rest of the DS vaccum. I also added support for realistic crystal placement and checks to avoid crystal overlaps.