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

Extended test on Geant4 geometry #27592

Merged
merged 4 commits into from Jul 27, 2019
Merged

Extended test on Geant4 geometry #27592

merged 4 commits into from Jul 27, 2019

Conversation

civanch
Copy link
Contributor

@civanch civanch commented Jul 24, 2019

PR description:

Extended test on Geant4 geometry is added, it is expected , that the test will help in Phase2 development and for DD4Hep migration. As a result of test few files are created with dumps of information:

  1. elements/material/cuts
  2. Geant4 regions
  3. Geant4 mother volumes (according to request it may be a world or some logical volume)
  4. Geant4 overlap check

Some classes are moved from SimG4Core/Application to SimG4Core/Geometry in order to have a correct dependecy between sub-libraries. Some files are renamed to avoid classes started from "G4".

Mainstream simulation should not be affected.

PR validation:

private

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27592/11017

  • This PR adds an extra 32KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27592/11018

  • This PR adds an extra 36KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @civanch (Vladimir Ivantchenko) for master.

It involves the following packages:

SimG4Core/Application
SimG4Core/Geometry
SimG4Core/PrintGeomInfo

@cmsbuild, @civanch, @mdhildreth can you please review it and eventually sign? Thanks.
@makortel 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

@civanch
Copy link
Contributor Author

civanch commented Jul 24, 2019

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 24, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/1605/console Started: 2019/07/24 12:05

@cmsbuild
Copy link
Contributor

-1

Tested at: 53fbbfe

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-01f731/1605/summary.html

I found follow errors while testing this PR

Failed tests: Build HeaderConsistency

  • Build:

I found compilation error when building:

Entering library rule at src/SimG4Core/Application/plugins
>> Compiling edm plugin /build/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-07-23-2300/src/SimG4Core/Application/plugins/OscarMTProducer.cc 
>> Compiling edm plugin /build/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-07-23-2300/src/SimG4Core/Application/plugins/OscarProducer.cc 
In file included from /build/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-07-23-2300/src/SimG4Core/Application/interface/OscarProducer.h:12:0,
                 from /build/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-07-23-2300/src/SimG4Core/Application/plugins/OscarProducer.cc:7:
/build/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-07-23-2300/poison/SimG4Core/Application/interface/CustomUIsession.h:1:2: error: #error THIS FILE HAS BEEN REMOVED FROM THE PACKAGE.
 #error THIS FILE HAS BEEN REMOVED FROM THE PACKAGE.
  ^~~~~
/build/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-07-23-2300/src/SimG4Core/Application/plugins/OscarProducer.cc: In constructor 'OscarProducer::OscarProducer(const edm::ParameterSet&)':
/build/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-07-23-2300/src/SimG4Core/Application/plugins/OscarProducer.cc:136:41: error: invalid use of incomplete type 'class CustomUIsession'
   m_UIsession.reset(new CustomUIsession());


@cmsbuild
Copy link
Contributor

Comparison not run due to Build errors (RelVals and Igprof tests were also skipped)

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27592/11028

  • This PR adds an extra 44KB to repository

@cmsbuild
Copy link
Contributor

Pull request #27592 was updated. @cmsbuild, @civanch, @mdhildreth can you please check and sign again.

@civanch
Copy link
Contributor Author

civanch commented Jul 24, 2019

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 24, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/1618/console Started: 2019/07/24 19:32

@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-01f731/1618/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 32
  • DQMHistoTests: Total histograms compared: 2628546
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2628228
  • DQMHistoTests: Total skipped: 317
  • 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

@civanch
Copy link
Contributor Author

civanch commented Jul 24, 2019

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

Copy link
Contributor

@fabiocos fabiocos left a comment

Choose a reason for hiding this comment

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

@civanch a very useful piece of work, I have just a couple of questions about the change of nature of some parameters

@@ -52,12 +52,20 @@
PhysicsTablesDirectory = cms.string('PhysicsTables'),
StorePhysicsTables = cms.bool(False),
RestorePhysicsTables = cms.bool(False),
CheckOverlap = cms.untracked.bool(False),
CheckGeometry = cms.bool(False),
Copy link
Contributor

Choose a reason for hiding this comment

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

@civanch why this and other verbosity flags are moved from untracked to tracked? What is the rationale behind it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fabiocos, "verbosity" parameters were added by different peoples and historically some are tracked, another untracked. The way how these parameters are used is well defined: only for debuging. They mainly are inside this base python file. Likely some may be removed, another may stay "tracked".

@@ -81,7 +89,7 @@
),
MagneticField = cms.PSet(
UseLocalMagFieldManager = cms.bool(False),
Verbosity = cms.untracked.bool(False),
Verbosity = cms.bool(False),
Copy link
Contributor

Choose a reason for hiding this comment

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

@civanch similar question here for instance

@@ -118,7 +126,7 @@
type = cms.string('SimG4Core/Physics/FTFP_BERT_EMM'),
DummyEMPhysics = cms.bool(False),
CutsPerRegion = cms.bool(True),
CutsOnProton = cms.untracked.bool(True),
CutsOnProton = cms.bool(True),
Copy link
Contributor

Choose a reason for hiding this comment

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

@civanch this I understand more

@fabiocos
Copy link
Contributor

@civanch if I want to recursively navigate into the volume hierarchy as before which settings should I provide now?

@civanch
Copy link
Contributor Author

civanch commented Jul 26, 2019

@fabiocos, in this test we may call Geant4 facility to check overlaps starting from some physical volume, it will go via full depth but due to migration to VecGeom this become extremely slow. With native Geant4 geometry this check takes ~20 minutes for full CMS and > 1 day for VecGeom. The possibility to limit depth allowing fast check on volume of interest. If depth=-1 full check will be applied.

In parallel here we have a possibility to make our own printout of physical and/or logical volumes, we define how deep we want to go. I need consult with Geant4 geometry experts if Geant4 internal facility still exist but experts are at vacations.

@fabiocos
Copy link
Contributor

@civanch ok, I see that with Depth set to -1 I may recover for instance a full scan of the FastTimerRegion. Of course it isn't fast, but it depends on what we need to check.

@fabiocos
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit ef0a99f into cms-sw:master Jul 27, 2019
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

3 participants