Skip to content
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

Migration to DD4hep for DT db Loader #32781

Merged
merged 11 commits into from Feb 26, 2021

Conversation

slomeo
Copy link
Contributor

@slomeo slomeo commented Feb 1, 2021

PR description:

This PR (only for DT) followed what I did for RPC in the PR #32102 .

PR validation:

Validation using "cmsRun CondTools/Geometry/test/dtgeometrywriter.py" (with DD4hep flag set False or True):
by checking printouts for both DD & DD4Hep build methods within the DTGeometryParsFromDD Class in order to compare some DetId parameters (see below):

//DD (in mm)

(0) DTGeometryParsFromDD - DD4HEP
(1) DD4HEP, Chamber DetId 574914560 1090 1255.5 181
(4) DD4HEP, Position 431.175 39.12 -533.35 // cm
(2) DD4HEP, Super Layer DetID 574922752 1063.2 1255.5 26.75
(4) DD4HEP, Position 419.425 37.52 -533.35 // cm
(4) DD4HEP, Position 417.475 37.52 -533.35 // cm
(3) DD4HEP, Layer DetID 574923776 1029.65 1199 5.75 1174
(4) DD4HEP, Position 418.775 37.52 -533.35 // cm
(3) DD4HEP, Layer DetID 574924800 1050.65 1199 5.75 1174
(4) DD4HEP, Position 420.075 37.52 -533.35 // cm
(3) DD4HEP, Layer DetID 574925824 1029.65 1199 5.75 1174
(4) DD4HEP, Position 421.375 37.52 -533.35 // cm
(3) DD4HEP, Layer DetID 574926848 1008.65 1199 5.75 1174

//DD4Hep (in mm)

(0) DTGeometryParsFromDD - DDD
(1) DDD, Chamber DetID 574914560 1090 1255.5 181
(4) DDD, Position 431.175 39.12 -533.35 // cm
(2) DDD, Super Layer DetID 574922752 1063.2 1255.5 26.75
(4) DDD, Position 419.425 37.52 -533.35 // cm
(4) DDD, Position 417.475 37.52 -533.35 // cm
(3) DDD, Layer DetID 574923776 1029.65 1199 5.75 1174
(4) DDD, Position 418.775 37.52 -533.35 // cm
(3) DDD, Layer DetID 574924800 1050.65 1199 5.75 1174
(4) DDD, Position 420.075 37.52 -533.35 // cm
(3) DDD, Layer DetID 574925824 1029.65 1199 5.75 1174
(4) DDD, Position 421.375 37.52 -533.35 // cm
(3) DDD, Layer DetID 574926848 1008.65 1199 5.75 1174

The DD part of the code has not been touched by this PR (except for the printouts). Added "/ dd4hep::mm;", where it was necessary, in the DD4Hep part of the code.

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

nothing special

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 1, 2021

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32781/20944

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 1, 2021

A new Pull Request was created by @slomeo (Sergio Lo Meo) for master.

It involves the following packages:

CondTools/Geometry
Geometry/DTGeometryBuilder

@civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @ggovi can you please review it and eventually sign? Thanks.
@mmusich, @fabiocos, @battibass 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

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 1, 2021

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32781/20945

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 1, 2021

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

@civanch
Copy link
Contributor

civanch commented Feb 1, 2021

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 1, 2021

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b960e1/12616/summary.html
COMMIT: 72d411e
CMSSW: CMSSW_11_3_X_2021-01-31-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/32781/12616/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

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

@ggovi
Copy link
Contributor

ggovi commented Feb 1, 2021

+1

@@ -18,18 +21,25 @@
#include "DataFormats/MuonDetId/interface/DTChamberId.h"
#include "DataFormats/GeometrySurface/interface/RectangularPlaneBounds.h"
#include "DataFormats/Math/interface/GeantUnits.h"

#include "DataFormats/GeometryVector/interface/Basic3DVector.h"
#include "CLHEP/Units/GlobalSystemOfUnits.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete GlobalSystemOfUnits.h. It should not be used. I'm not sure what it is needed for in the file, but GeantUnits.h should provide the same functionaliy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cvuosalo : done


if (fromDD4Hep_) {
edm::ESTransientHandle<cms::DDCompactView> pDD;
es.get<IdealGeometryRecord>().get(pDD);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

es.get() is deprecated. If you can, it would be good to change this code to the recommended way:
https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideHowToGetDataFromES

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cvuosalo : yes but, before perform this change, I'd prefer to commit the other modifications, I made for the dd4hep::mm, that you requested then, if the tests and histos comparison will be ok I'll proceed with this point.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the EventsSetup changes can be made in a later PR if you wish.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cvuosalo : I'll try to do it in this PR after the test will be finished

@cvuosalo
Copy link
Contributor

cvuosalo commented Feb 1, 2021

@slomeo The values for DD4hep must match those with DD. When DD is using millimeters, which is most cases, then with DD4hep you have to convert to mm with / dd4hep::mm. In the print-outs, the numbers must match.

@slomeo
Copy link
Contributor Author

slomeo commented Feb 2, 2021

@slomeo The values for DD4hep must match those with DD. When DD is using millimeters, which is most cases, then with DD4hep you have to convert to mm with / dd4hep::mm. In the print-outs, the numbers must match.

@cvuosalo : done, please take a look to the summary at the top of this PR.

@cmsbuild
Copy link
Contributor

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

@smuzaffar
Copy link
Contributor

please test
input for wf 4.76 should be fixed now.

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b960e1/12916/summary.html
COMMIT: 115cd92
CMSSW: CMSSW_11_3_X_2021-02-15-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/32781/12916/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-b960e1/11634.911_TTbar_14TeV+2021_DD4hep+TTbar_14TeV_TuneCP5_GenSim+Digi+Reco+HARVEST+ALCA

Summary:

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

@cvuosalo
Copy link
Contributor

+1

@slomeo
Copy link
Contributor Author

slomeo commented Feb 17, 2021

@cvuosalo when this PR will be merged, I'll start to work on a second validation for PRs #32102 and #32001 (RPC & CSC) by the same tests I did for DT

@cvuosalo
Copy link
Contributor

@slomeo Yes, it would be good to test the CSC and RPC loaders, too. Thanks.

@silviodonato
Copy link
Contributor

kind reminder @ggovi

@slomeo
Copy link
Contributor Author

slomeo commented Feb 20, 2021

@ggovi : do you see any problems in this PR?

@ggovi
Copy link
Contributor

ggovi commented Feb 26, 2021

+1

@cmsbuild
Copy link
Contributor

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
Copy link
Contributor

qliphy commented Feb 26, 2021

+1

1 similar comment
@qliphy
Copy link
Contributor

qliphy commented Feb 26, 2021

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants