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: update etlv5 setup to fit in D75-D76 envelope without overlaps, move unit tests to D76 #32840
Conversation
…e reference scenario for unit tests
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32840/21052
|
A new Pull Request was created by @fabiocos (Fabio Cossutti) for master. It involves the following packages: Geometry/MTDCommonData @perrotta, @civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @srimanob, @kpedro88, @slava77, @jpata can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
please test with cms-data/Geometry-TestReference#5 |
data packages cannot be directly tested by the bot without an update of cmsdist, hence the failure of |
-1 Failed Tests: UnitTests RelVals-INPUT Unit TestsI found errors in the following unit tests: ---> test GeometryMTDGeometryBuilderTestDriver had ERRORS ---> test GeometryMTDNumberingBuilderTestDriver had ERRORS ---> test GeometryMTDCommonDataTestDriver had ERRORS RelVals-INPUTComparison SummarySummary:
|
+1 |
@fabiocos By the way, why don't we use D76 as a baseline for unit test? |
@srimanob the failure in unit test is simply due to the fact that the output of the test (D75 with revised etlv5) is compared with the reference from D50, I have provided the new references in cms-data/Geometry-TestReference#5 but they cannot be trivially used as done in my command, the need to be imported in a cmsdist update (@smuzaffar do you confirm)? So this PR should be updated in parallel with cms-data and cmsdist, otherwise you will get annoying failures in any IB and/or PR test just because of this. The failure in comparison seems to have nothing to do with my PR (no MTD in 2021 geometry, I hope...). I am not familiar with this new matrix input test check, but it looks to be again unrelated to my updates. |
@srimanob concerning the new baseline to be used, up to you to tell. From the MTD point of view D75 and D76 should be identical, if you prefer D76 I can easily modify this PR. I expect that soon we will want to use D77, but presumably it is too early. In any case, as I mentioned to @silviodonato , it would be good to have a test wf in the short matrix for the new baseline asap, so as to have a direct monitoring of it in any possible check. Please let me know |
@fabiocos , bot can test data packages (without cmsdist update). The data files from cms-data/Geometry-TestReference#5 should be available in the bot PR tests |
@smuzaffar thanks for recalling me, so let me check once more the test failure. The cmsdist will be done at the integration time, right? |
@srimanob ok, I understand the issue, I did not check carefully enough the test output: the unit test looks successful in the DDD part, while it fails in the DD4hep part because I only partly adapted the xml reference name. I am going to fix it, please confirm that you prefer D76 as new baseline. |
please test 34234.0 with cms-data/Geometry-TestReference#5 |
@silviodonato as far as I can see the available test for D76 is 34634.0
, anyway D75 and D76 are identical as far as MTD is concerned |
@silviodonato in my local test 34634.0 runs smoothly |
please test workflow 34234.0,34634.0 with cms-data/Geometry-TestReference#5 |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-199547/12804/summary.html Comparison Summary@slava77 comparisons for the following workflows were not done due to missing matrix map:
Summary:
|
@cms-sw/geometry-l2 @cms-sw/reconstruction-l2 do you have any comments? |
+1
|
+1 |
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) |
+1 |
Could this PR be causing a failure in 33434.0 step 1? See #32882 for more details.
|
This PR makes any scenario that uses Either this PR should be reverted, or this version mismatch should be fixed. |
Since we plan to keep old geometry, i.e. D72, should we make v6/etl.ml (instead of updating v5) to be used with v5/caloBase.xml? @fabiocos Thanks. |
PR description:
In this PR the MTD geometry scenario I13 (with ETL v5), as recently added to scenario D75, is updated (by @martatornago) to fit without overlaps in the external envelope available. Some ad hoc z shifts are removed, and the x-y order of the module layout is reviewed for a consistent treatment fo both disc faces. x-y sensor positions are unchanged overall, but the assignment of ETLDetId changes.
The scenario D75 is now used as the baseline for unit tests in both the Geometry and the RecOMTD/DetLayers packages. A corresponding update of the references used by Geometry unit tests is proposed in cms-data/Geometry-TestReference#5 , they need to be integrated at the same time in order to avoid failures.
PR validation:
Unit tests run, and a visual inspection shows the expected results. The upgrade test workflow 34234.0 for scenario D75 runs smoothly. The Geant4 overlap test does not show any residual issue in
EndcapTimingLayer
.