-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
MTD geometry: fix scenario I14 for material budget studies #34625
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34625/24211
|
A new Pull Request was created by @fabiocos (Fabio Cossutti) for master. It involves the following packages:
@civanch, @Dr15Jones, @ahmad3213, @kmaeshima, @cvuosalo, @andrius-k, @kpedro88, @ianna, @cmsbuild, @makortel, @srimanob, @jfernan2, @mdhildreth, @ErnestaP, @rvenditti can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
please test |
-1 Failed Tests: UnitTests RelVals-INPUT Unit TestsI found errors in the following unit tests: ---> test materialBudgetTrackerPlots had ERRORS ---> test materialBudgetHGCalPlots had ERRORS RelVals-INPUT
Expand to see more relval errors ...
Comparison SummarySummary:
|
I do not see how the listed errors might have anything to do with this PR, the failing unit tests are the same failing in the IB, and should not be affected by these changes |
+1 |
+Upgrade Failure in the unit test seems not to relate to this PR. |
+1 |
This pull request is fully signed and it will be integrated in one of the next master IBs (but tests are reportedly failing). This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy, @perrotta (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
merge |
@@ -154,6 +154,11 @@ | |||
<rMaterial name="materials:Air"/> | |||
</MaterialFraction> | |||
</CompositeMaterial> | |||
<CompositeMaterial name="Borated_Polyethyl." density="950*mg/cm3" symbol=" " method="mixture by weight"> |
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.
Why is this seemingly duplicate material introduced? It has the same density as materials:Borated Polyethyl.
.
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.
@cvuosalo this is essentially a workaround to let the material budget validation code parsing without issues the tables with the material names, it is unclear to me how to handle blanks in the middle (it was a bad idea to introduce them in first place, I would say). I mimicked what done already for other similar situations, see for instance
https://cmssdt.cern.ch/lxr/source/Geometry/CMSCommonData/data/materials.xml#1157
If you have a better solution to suggest I will be happy to consider it, I do not like this solution either, but "it just works".
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.
@fabiocos Thanks, I understand now. It is unfortunate that we are stuck with this name containing a space, but I see it is widely used, so it would be some effort to change.
PR description:
This is a minimal completion of the input provided to @bsunanda for #34481 to allow MTD material budget studies with scenario I14.
PR validation:
Code runs and provides material budget plots, as presented by @martatornago in https://indico.cern.ch/event/1057473/contributions/4444289/attachments/2279146/3872237/ETLGeometryUpdate.pdf