ETL v5 (D73) x,y axes swapped and hardcoded offset removed

[martatornago]

PR description:

X and y axes, along which ETL modules and service hybrids are placed, have been swapped, and the phi angle of the disks has been properly adapted. In addition, hardcoded offsets previously introduced have been removed. These changes fix the errors present in the previous version, according to what has been presented by N. Koss at the last CMS week

PR validation:

Visualization of the geometry with Fireworks
Verified there are no overlaps with g4OverlapCheck_cfg.py
Check of the modules position by comparing x,y,z coordinates with the ones in the log produced by testMTDinDD4hep.py

if this PR is a backport please specify the original PR and why you need to backport that PR:

Before submitting your pull requests, make sure you followed this checklist:

• verify that the PR is really intended for the chosen branch
• verify that changes follow CMS Naming, Coding, And Style Rules
• verify that the PR passes the basic test procedure suggested in the CMSSW PR instructions

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32472/20378

• This PR adds an extra 20KB to repository

[cmsbuild]

A new Pull Request was created by @martatornago (martatornago) for master.

It involves the following packages:

Geometry/MTDCommonData

@civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @kpedro88 can you please review it and eventually sign? Thanks.
@fabiocos this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

[fabiocos]

please test

since we do not have tests for scenario D73 in the short matrix I do not expect any visible effect here

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-81494c/11610/summary.html
CMSSW: CMSSW_11_3_X_2020-12-14-1100/slc7_amd64_gcc900

Comparison Summary

Summary:

• No significant changes to the logs found
• Reco comparison results: 7 differences found in the comparisons
• DQMHistoTests: Total files compared: 36
• DQMHistoTests: Total histograms compared: 2747284
• DQMHistoTests: Total failures: 12
• DQMHistoTests: Total nulls: 1
• DQMHistoTests: Total successes: 2747249
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.004 KiB( 35 files compared)
• DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
• Checked 153 log files, 37 edm output root files, 36 DQM output files

[fabiocos]

The reported DQM differences have nothing to do with this PR

[fabiocos]

I suggest to put this PR on hold untile we have a private validation test of a full D73 workflow with it

[kpedro88]

It's acceptable to have a PR that just adds the geometry without a complete scenario, if that's useful for development.

However, this PR doesn't even add a geometry scenario, so its utility seems very limited.

[fabiocos]

@kpedro88 as you can see this is just ETL for scenario D73, this PR is fixing a global 90-degree rotation that I realized in my checks on reco geometry and was confirmed by the updated full validation plots by @gsorrentino18 (see https://indico.cern.ch/event/984445/contributions/4146432/attachments/2161556/3647170/MTD-DPG%2011_12_2020.pdf slide 7, module rows should run along x and not y). While @martatornago has performed standalone checks comparing the dumped positions with the expected ones, I'd like to run a complete D73 test myself with it before merging.

[fabiocos]

BTW it would make sense to backport this into 11_2_X, if we want to use that release for any meaningful test

[kpedro88]

Ah sorry, I had forgotten D73 was already introduced.

[cvuosalo]

The Constants sections starts with this line:

  <ConstantsSection label="etl.xml" eval="true/false">

I think the eval part should be eval="true".
Also, for clarity, in the Constants section it would be good to give units for all lengths, that is "*mm".

[fabiocos]

@cvuosalo I agree for the eval=true, @icosivi is there a reason to chose the other form?

Concerning the units, they are all given in the other statements, I am afraid that if we add them also in the constants we might have some mess (in case they are not trivially 1).

[fabiocos]

@martatornago was the output of the DDD and DD4hep versions of the geometry compared? are they identically equal?

[icosivi]

@fabiocos no, there is no reason to choose the other form. We forgot to choose "true" in that line.

[martatornago]

@fabiocos I’ve just checked again with both DDD and DD4hep and they are identical, yes.

…

Il giorno 15 dic 2020, alle ore 10:01, Fabio Cossutti ***@***.***> ha scritto: @martatornago <https://github.com/martatornago> was the output of the DDD and DD4hep versions of the geometry compared? are they identically equal? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#32472 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AO5I3LT2TN6CZ7EN44LYNGDSU4QYNANCNFSM4U2Y7CNA>.

-- ------------------------ Indirizzo istituzionale di posta elettronica degli studenti e dei laureati dell'Università degli Studi di TorinoOfficial  University of Turin email address for students and graduates 

[fabiocos]

@martatornago thanks, I anyway suggest to fix the eval according to @cvuosalo comment

[cmsbuild]

Pull request #32472 was updated. @civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @kpedro88 can you please check and sign again.

[kpedro88]

please test

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-81494c/11690/summary.html
CMSSW: CMSSW_11_3_X_2020-12-15-1100/slc7_amd64_gcc900

Comparison Summary

Summary:

• No significant changes to the logs found
• Reco comparison results: 4 differences found in the comparisons
• DQMHistoTests: Total files compared: 36
• DQMHistoTests: Total histograms compared: 2747284
• DQMHistoTests: Total failures: 7
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 2747255
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 35 files compared)
• Checked 153 log files, 37 edm output root files, 36 DQM output files

[cvuosalo]

@fabiocos Units for lengths in the XML files can be added in a later PR.

[cvuosalo]

+1

[fabiocos]

@cvuosalo sorry, where do you see missing units? The constants are pure numbers, but then the units are added where constants are used, is this invalid? The DD4hep geometry output is identical to the DDD one according to the dumps.

[cvuosalo]

@fabiocos The following sound like lengths to me:

  <Constant name="ETLrmax" value="[caloBase:Rmax100]"/>
    <Constant name="Disc_thickness" value="27.9"/>
    <Constant name="Al_Disc_thickness" value="7.24"/>
    <Constant name="Disc_Rmin" value="310"/>
    <Constant name="Disc_Rmax" value="1190"/>   
    <Constant name="DeltaY" value="0.5"/>

Isn't a thickness a length? I assume it would be measured in cm or mm.
For clarity, since the values are defined here in the XML, it would be good to define them with units, e.g.:

<Constant name="Disc_thickness" value="27.9*mm"/>

This specification is for readability. If I want to find out the units for a constant, it would seem natural to go to the source XML. If the XML does not have the units, then I have to search around in the source code to try to figure out what the units are.
Units conventions are a source of confusion in the Geometry code. We use both the Geant4 and ROOT units conventions, and we switch back and forth between them for simulation, the DB, and reco. I would like to encourage practices to keep the units as clear as possible.

[fabiocos]

@cvuosalo you are correct, but the units are specified in the instructions where these parameters are used. So strictly speaking I would say that the XML gives a correct result, unless units are missing somewhere. A different story might be using these constants somewhere else without specifying the unit.

One may move units from the body to the constant definition, but this would require sometime, and I would factorize it from this fix

[kpedro88]

+upgrade

[cmsbuild]

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

[qliphy]

I'd like to run a complete D73 test myself with it before merging.

@fabiocos Let's know when the tests are done. Thanks!

[gsorrentino18]

I tested this PR with 200 TTbar events in the D73 scenario (WF 33434).
The RECO Occupancy plots below have been obtained with the last version of the ETL validation code (see PR #32476) and they show the expected 90-degrees rotation of the ETL modules. I observed the same rotation at SIM and DIGI level as well.

[fabiocos]

thanks to the test of @gsorrentino18 I would say that a basic validation of this PR has been done, and I consider it enough to merge it. As discussed above, @martatornago will provide a further update addressing the technical comments by @cvuosalo and a possible further update of some design details.
@qliphy @silviodonato as far as I am concerned this PR can be merged at your earliest convenience. We will also need its backport to 11_2_X, but I would suggest to wait that the further updates are merged as well

[silviodonato]

@fabiocos ok, thanks
@martatornago I see you have changed <Constant name="Disc_Rmin" value="300"/> to <Constant name="Disc_Rmin" value="310"/> .
Have you done it on purpose?

[martatornago]

@silviodonato yes, actually I reduced the value from 310mm to 300mm because the inner radius of the geometry has changed (in fact with 310mm there were some overlaps)

[silviodonato]

+1