search

cms-sw / cmssw

CMS Offline Software
http://cms-sw.github.io/
Apache License 2.0
1.09k stars 4.32k forks source link

MTD geometry: update MTDGeometry test, use only DD4hep backend, add ETL pixel position test #46683

Closed fabiocos closed 1 day ago

fabiocos commented 1 week ago

PR description:

Complete revision of the (unit) test suite for Geometry/MTDGeometryBuilder:

Only the DD4hep version of the test is now run, since MTDGeometry is independent on the backend, provided the MTDNumberingBuilder output is the same.

The filtering of volumes has been optimized to prevent the navigation of the full volume tree.

PR validation:

The code runs for scenario D110, providing the expected output. The reference needs to be updated in the cms-data area to have a successful completion of the unit test.

cmsbuild commented 1 week ago

cms-bot internal usage

cmsbuild commented 1 week ago

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46683/42617

cmsbuild commented 1 week ago

A new Pull Request was created by @fabiocos for master.

It involves the following packages:

@Dr15Jones, @Moanwar, @bsunanda, @civanch, @cmsbuild, @kpedro88, @makortel, @mdhildreth, @srimanob, @subirsarkar can you please review it and eventually sign? Thanks. @bsunanda, @martinamalberti this is something you requested to watch as well. @antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

fabiocos commented 1 week ago

please test with https://github.com/cms-data/Geometry-TestReference/pull/16

cmsbuild commented 1 week ago

-1

Failed Tests: UnitTests Size: This PR adds an extra 24KB to repository Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8b4aac/42774/summary.html COMMIT: db2346d1a92446905ec3a139aa0658777376cb16 CMSSW: CMSSW_14_2_X_2024-11-12-1100/el8_amd64_gcc12 User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/46683/42774/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

You can see more details here: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8b4aac/42774/git-recent-commits.json https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8b4aac/42774/git-merge-result

Unit Tests

I found 1 errors in the following unit tests:

---> test TestDQMGUIUpload had ERRORS

Comparison Summary

There are some workflows for which there are errors in the baseline: 2024.101001 step 1 2024.202001 step 1 2024.303001 step 1 The results for the comparisons for these workflows could be incomplete This means most likely that the IB is having errors in the relvals.The error does NOT come from this pull request

Summary:

fabiocos commented 1 week ago

Failures appear unrelated to this PR

cmsbuild commented 1 week ago

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46683/42622

cmsbuild commented 1 week ago

Pull request #46683 was updated. @Dr15Jones, @Moanwar, @bsunanda, @civanch, @cmsbuild, @kpedro88, @makortel, @mdhildreth, @srimanob, @subirsarkar can you please check and sign again.

fabiocos commented 1 week ago

please test with https://github.com/cms-data/Geometry-TestReference/pull/16

cmsbuild commented 1 week ago

-1

Failed Tests: UnitTests Size: This PR adds an extra 24KB to repository Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8b4aac/42778/summary.html COMMIT: a07ea53e1386725b09a250d3aea94022041102e3 CMSSW: CMSSW_14_2_X_2024-11-12-1100/el8_amd64_gcc12 User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/46683/42778/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

You can see more details here: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8b4aac/42778/git-recent-commits.json https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8b4aac/42778/git-merge-result

Unit Tests

I found 1 errors in the following unit tests:

---> test TestDQMGUIUpload had ERRORS

Comparison Summary

There are some workflows for which there are errors in the baseline: 2024.101001 step 1 2024.202001 step 1 2024.303001 step 1 The results for the comparisons for these workflows could be incomplete This means most likely that the IB is having errors in the relvals.The error does NOT come from this pull request

Summary:

fabiocos commented 1 week ago

@cms-sw/geometry-l2 @cms-sw/upgrade-l2 the errors have nothing to do with this PR, any comment?

Moanwar commented 1 week ago

+Upgrade

fabiocos commented 4 days ago

@cms-sw/geometry-l2 comments?

fabiocos commented 2 days ago

please test with https://github.com/cms-data/Geometry-TestReference/pull/16

cmsbuild commented 2 days ago

+1

Size: This PR adds an extra 12KB to repository Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8b4aac/42963/summary.html COMMIT: a07ea53e1386725b09a250d3aea94022041102e3 CMSSW: CMSSW_14_2_X_2024-11-19-2300/el8_amd64_gcc12 User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/46683/42963/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

civanch commented 1 day ago

+1

cmsbuild commented 1 day ago

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. @sextonkennedy, @antoniovilela, @rappoccio, @mandrenguyen (and backports should be raised in the release meeting by the corresponding L2) Notice This PR was tested with additional Pull Request(s), please also merge them if necessary: cms-data/Geometry-TestReference#16

mandrenguyen commented 1 day ago

+1

fabiocos commented 22 hours ago

I see that the unit test is failing in the IB, while it was successful in the PR test. I wonder whether it depends on details of the architecture, as this appears to be linked to rounding. I will fix that by reducing the number of digits used to the strict minimum needed

makortel commented 20 hours ago

I see that the unit test is failing in the IB, while it was successful in the PR test. I wonder whether it depends on details of the architecture, as this appears to be linked to rounding. I will fix that by reducing the number of digits used to the strict minimum needed

Is the test failure seen only in MULTIARCHS IB? If it's only there, then this sounds like a x86-64-v2 vs v3 difference (FMA being the first suspect coming to mind);

fabiocos commented 6 hours ago

@makortel no, I see it also in aarch64. There was a similar issue in the RecoMTD/DetLayers test in the past, I'll adopt the same approach as there, and test it also on ARM to be sure