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

Start conversion to using user-defined literals for Geometry #24509

Merged
merged 4 commits into from Oct 11, 2018

Conversation

cvuosalo
Copy link
Contributor

It is hoped that replacing constants from CLHEP/Units/GlobalSystemOfUnits.h with user-defined literals from DetectorDescription/Core/interface/DDUnits.h will improve performance by eliminating unnecessary floating point operations and conversions. This PR is a preliminary step in a campaign to convert the Geometry code to employ user-defined literals.

The changes in this PR do not appreciably change performance since the affected code is run only once at the start of simulation, but it does show the sort of changes entailed in the conversion to user-defined literals.

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @cvuosalo (Carl Vuosalo) for master.

It involves the following packages:

DetectorDescription/Core
Geometry/MuonCommonData

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

cms-bot commands are listed here

@cvuosalo
Copy link
Contributor Author

Note that two small changes in program behavior have been implemented. First, the old code was truncating fractional values in degrees of rotations, so values of n.0 to n.899999 were truncated to n.0 degrees (where n could be positive or negative). On the advice of @ianna, this truncation has been eliminated, but it might cause a very small change in the value of rotations.
Second, a test for a non-zero rotation in the DDMuonAngular code was changed from requiring std::abs(rotation) >= 0.9_deg to std::abs(rotation) >= 1.0_deg, so rotations of 0.9 degrees in the old code will be skipped by the new code.

@cvuosalo
Copy link
Contributor Author

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 12, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/30383/console Started: 2018/09/12 23:15

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-24509/30383/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 10438 differences found in the comparisons
  • DQMHistoTests: Total files compared: 32
  • DQMHistoTests: Total histograms compared: 3147363
  • DQMHistoTests: Total failures: 15835
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3131330
  • DQMHistoTests: Total skipped: 197
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
  • Checked 133 log files, 14 edm output root files, 32 DQM output files

@ianna
Copy link
Contributor

ianna commented Sep 17, 2018

hold

@cmsbuild
Copy link
Contributor

Pull request has been put on hold by @ianna
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

@cmsbuild cmsbuild added the hold label Sep 17, 2018
@cvuosalo
Copy link
Contributor Author

@ianna: I will restore the truncation of rotation angle to the nearest degree for the GEM code and test to see if the discrepancies disappear.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-24509/30625/summary.html

Comparison Summary:

  • You potentially added 110 lines to the logs
  • Reco comparison results: 5013 differences found in the comparisons
  • DQMHistoTests: Total files compared: 32
  • DQMHistoTests: Total histograms compared: 3146876
  • DQMHistoTests: Total failures: 6221
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3140458
  • DQMHistoTests: Total skipped: 197
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
  • Checked 133 log files, 14 edm output root files, 32 DQM output files

@ianna
Copy link
Contributor

ianna commented Sep 26, 2018

@cvuosalo - geometry validation seems to be fine for the RPC geometry. DTs do show superficial differences in a norm vector, for example:

normVect  (-1,5.17598e-16,1.33963e-16)

vs

normVect  (-1,5.17598e-16,1.8941e-16)

To reproduce the results, please:

git cms-addpkg Validation/Geometry
./Validation/Geometry/test/dddvsdb/runDDDvsDBGeometryValidation.sh  auto:upgrade2019 GeometryExtended2019

Please, see:
https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideDDDversusDataBaseValidation

@ianna
Copy link
Contributor

ianna commented Sep 26, 2018

The DT reconstruction geometry has not been updated since CMSSW 5.0. It differs from the currently built from 10.3.x pre-release:
https://cms-conddb.cern.ch/cmsDbBrowser/diff/Prod/tags/DTRECO_Geometry_103YV1/DTRECO_Geometry_50YV3
These differences are unrelated to this PR. It could be related to moving from float to double.

@ianna
Copy link
Contributor

ianna commented Sep 27, 2018

unhold

@cmsbuild cmsbuild removed the hold label Sep 27, 2018
@ianna
Copy link
Contributor

ianna commented Sep 27, 2018

Testing the DT reco geometry against 103X_postLS2_design_Candidate_2018_09_27_08_17_34 GT candidate queue shows no differences.

@ianna
Copy link
Contributor

ianna commented Sep 27, 2018

+1

There are no differences in either 2019 or 2023D17. The other 2023D* scenario differences could be due to low statistics.

@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. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

@fabiocos
Copy link
Contributor

@ianna 2018 and older setups use the DB snapshot, so you rely on upgrade scenarios for direct DD validation, am I correct?

@ianna
Copy link
Contributor

ianna commented Oct 1, 2018

@fabiocos - no, the DD validation is done by comparison XML vs DB vs local DB with a script:

git cms-addpkg Validation/Geometry
./Validation/Geometry/test/dddvsdb/runDDDvsDBGeometryValidation.sh  auto:upgrade2019 GeometryExtended2019

There are no differences in the 2019 GEM reco geometry:
https://cms-conddb-prod.cern.ch/cmsDbBrowser/diff/Prod/tags/GEMRECO_Geometry_103YV1/GEMRECO_Geometry_10YV4

@fabiocos
Copy link
Contributor

+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

5 participants