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

Update DD4Hep to latest on master #6554

Merged
merged 3 commits into from Jan 25, 2021

Conversation

mrodozov
Copy link
Contributor

No description provided.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @mrodozov (Mircho Rodozov) for branch IB/CMSSW_11_3_X/master.

@cmsbuild, @smuzaffar, @mrodozov can you please review it and eventually sign? Thanks.
cms-bot commands are listed here

@mrodozov
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

-1

Failed Tests: UnitTests RelVals
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e91374/12291/summary.html
COMMIT: ca04fd5
CMSSW: CMSSW_11_3_X_2021-01-14-1100/slc7_amd64_gcc900

Unit Tests

I found errors in the following unit tests:

---> test testDD4hepFilteredView had ERRORS
---> test GeometryDTGeometryBuilderTestDriver had ERRORS
---> test GeometryMTDGeometryBuilderTestDriver had ERRORS
---> test GeometryMTDNumberingBuilderTestDriver had ERRORS
---> test GeometryMTDCommonDataTestDriver had ERRORS

RelVals

11624.911_TTbar_13+2021_DD4hep+TTbar_13TeV_TuneCUETP8M1_GenSim+Digi+Reco+HARVEST+ALCA/step1_TTbar_13+2021_DD4hep+TTbar_13TeV_TuneCUETP8M1_GenSim+Digi+Reco+HARVEST+ALCA.log

@mrodozov
Copy link
Contributor Author

@cvuosalo @ianna this needs cmssw update

@ianna
Copy link

ianna commented Jan 15, 2021

@mrodozov - yes, thanks!
@cvuosalo - the recent changes to testDDFilteredView should be removed - the reference values are already expecting a correct result from an evaluator:

Get attl from hf as double values:
attl 8.09653e-05 == 0.000809653
F

DDFilteredView.cppunit.cc:100:Assertion
Test name: testDDFilteredView::checkFilteredView
assertion failed
- Expression: abs(i - refdattl_[count]) < 10e-6

Failures !!!
Run: 1   Failure total: 1   Failures: 1   Errors: 0

---> test testDD4hepFilteredView had ERRORS

@cvuosalo
Copy link

@mrodozov How is CMSSW being compiled against this version of DD4hep? When CMSSW code includes DD4hep/DDParsers/include/Evaluator/DD4hepUnits.h, the HAVE_GEANT4_UNITS compiler directive needs to be set. Is that what happens in this PR?

@cvuosalo
Copy link

@ianna In the line:

attl 8.09653e-05 == 0.000809653

The value on the left is the calculated value, and on the right it is the reference value. Here, the calculated value is too small. The current code does this:

 i *= dd4hep::cm;  // Convert DD4hep units to /cm

For Geant4 units dd4hep::cm = 10, but I don't think CMSSW is including the Geant4 version of DD4hepUnits.h. I think CMSSW is getting the ROOT version of DD4hepUnits.h, dd4hep::cm = 1, and the value returned by DDFilteredView::get() is multiplied by 1 instead of 10 and turns out too small.

@mrodozov
Copy link
Contributor Author

please test

@mrodozov
Copy link
Contributor Author

@cmsbuild
Copy link
Contributor

-1

Failed Tests: UnitTests RelVals
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e91374/12314/summary.html
COMMIT: ca04fd5
CMSSW: CMSSW_11_3_X_2021-01-17-2300/slc7_amd64_gcc900

Unit Tests

I found errors in the following unit tests:

---> test DetectorDescriptionDDCMSTestDriver had ERRORS
---> test testDD4hepCompactView had ERRORS
---> test testDD4hepDDSolid had ERRORS
---> test testDD4hepFilteredViewFind had ERRORS
---> test testDD4hepFilteredView had ERRORS
---> test testDD4hepFilteredViewGet had ERRORS
---> test testDD4hepFilteredViewFirstChild had ERRORS
---> test testDD4hepFilteredViewLevel had ERRORS
---> test testDD4hepFilteredViewGoTo had ERRORS
---> test GeometryDTGeometryBuilderTestDriver had ERRORS
---> test GeometryMuonCommonDataTestDriver had ERRORS
and more ...

RelVals

11624.911_TTbar_13+2021_DD4hep+TTbar_13TeV_TuneCUETP8M1_GenSim+Digi+Reco+HARVEST+ALCA/step1_TTbar_13+2021_DD4hep+TTbar_13TeV_TuneCUETP8M1_GenSim+Digi+Reco+HARVEST+ALCA.log

----- Begin Fatal Exception 18-Jan-2021 12:44:04 CET-----------------------
An exception of category 'StdException' occurred while
   [0] Processing global begin Run run: 1
   [1] Calling method for module OscarMTProducer/'g4SimHits'
   [2] Using EventSetup component DDCompactViewESProducer/'' to make data DDCompactView/'' in record IdealGeometryRecord
   [3] Running EventSetup component DDDetectorESProducer/'
Exception Message:
A std::exception was thrown.
dd4hep: Failed to locate plugin to interprete files of type "DDDefinition" - no factory:DDDefinition_XML_reader. 		No factory with name Create(DDDefinition_XML_reader) for type DDDefinition_XML_reader found.
		Please check library load path and/or plugin factory name.
dd4hep: while parsing /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmsdist/6554/12314/CMSSW_11_3_X_2021-01-17-2300/src/Geometry/CMSCommonData/data/dd4hep/cmsExtendedGeometry2021.xml
dd4hep: with plugin:DD4hep_CompactLoader
----- End Fatal Exception -------------------------------------------------

@smuzaffar
Copy link
Contributor

smuzaffar commented Jan 19, 2021

@mrodozov , please update https://github.com/cms-sw/cmsdist/blob/IB/CMSSW_11_3_X/master/dd4hep-toolfile.spec and add something like

<flags cppdefines="DD4HEP_USE_GEANT4_UNITS=1"/>

add this line here https://github.com/cms-sw/cmsdist/blob/IB/CMSSW_11_3_X/master/dd4hep-toolfile.spec#L24

@smuzaffar
Copy link
Contributor

lets fix the toolfile and re-run the tests and see if we unit tests still fail

@davidlange6
Copy link
Contributor

@mrodozov , please update https://github.com/cms-sw/cmsdist/blob/IB/CMSSW_11_3_X/master/dd4hep-toolfile.spec and add something like

<flags cppdefines="DD4HEP_USE_GEANT4_UNITS=1"/>

add this line here https://github.com/cms-sw/cmsdist/blob/IB/CMSSW_11_3_X/master/dd4hep-toolfile.spec#L24

having this defined in two places seems a plan for problems. Why can't dd4hep define this internally in a header according to the way make is run?

@smuzaffar
Copy link
Contributor

This is a build time configuration parameter. So either we can patch dd4hep (in our spec) to set it in side one of the dd4hep header (which will avoid the duplication) or we have to use the scram toolfiles

@smuzaffar
Copy link
Contributor

smuzaffar commented Jan 19, 2021

ah right ( https://github.com/cms-sw/cmsdist/pull/6554/files#diff-71d575042aa0c5063c7a3b3456d09216a283b3805936d7ee23702fac27a752e9R27 ), I did not noitce that it is set via cmake ( i thought we were setting it at make time). Yes dd4hep should set it in one of its header

@mrodozov
Copy link
Contributor Author

mrodozov commented Jan 19, 2021

actually the macro to be setup is different from the cmake flag. I made the same mistake :) The C++ macro to be defined is called
HAVE_GEANT4_UNITS
while the cmake variable is called
DD4HEP_USE_GEANT4_UNITS
HAVE_GEANT4_UNITS in the toolfile then ?
I have a local setup to tst until it works, then I'm adding it to the toolfile

@smuzaffar
Copy link
Contributor

@mrodozov , I copuld not find any ref to HAVE_GEANT4_UNITS but I see that DD4HEP_USE_GEANT4_UNITS is the marco used in few dd4hep files e.g. https://github.com/AIDASoft/DD4hep/blob/master/DDParsers/include/Evaluator/DD4hepUnits.h#L128
I would suggest that for now update the toolfile to set the DD4HEP_USE_GEANT4_UNITS cppdefine so that we can test the dd4hep changes

Currently dd4hep record this marco in the its generated cmake files

./cmake/DD4hepConfig-targets.cmake:  INTERFACE_COMPILE_DEFINITIONS "BOOST_SPIRIT_USE_PHOENIX_V3;DD4HEP_USE_GEANT4_UNITS=1"

but as cmssw do not use cmake so that is no use of us. I would suggest that ask dd4hep developers to define this macro in their header file if dd4hep is build with DD4HEP_USE_GEANT4_UNITS ON. this will allow projects without cmake support to use it without re-definiing it in their build system

@cmsbuild
Copy link
Contributor

-1

Failed Tests: UnitTests RelVals
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e91374/12395/summary.html
COMMIT: ebac4e4
CMSSW: CMSSW_11_3_X_2021-01-20-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmsdist/6554/12395/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found errors in the following unit tests:

---> test testDD4hepFilteredView had ERRORS
---> test GeometryDTGeometryBuilderTestDriver had ERRORS

RelVals

11624.911_TTbar_13+2021_DD4hep+TTbar_13TeV_TuneCUETP8M1_GenSim+Digi+Reco+HARVEST+ALCA/step1_TTbar_13+2021_DD4hep+TTbar_13TeV_TuneCUETP8M1_GenSim+Digi+Reco+HARVEST+ALCA.log

@mrodozov
Copy link
Contributor Author

@cvuosalo @ianna the flag is properly set and failures has nothing to do with it, please advise. thank you

@cvuosalo
Copy link

@ianna @civanch Advice from @ianna and the DD4hep team is needed. I sent email about the question. It's not clear to me whether the fault is in our unit test or in DD4hep.

@cvuosalo
Copy link

After discussion, we decided that the failing unit tests need a fix. I will work on that.

@cvuosalo
Copy link

@mrodozov PR #32721 should fix the DDFilteredView error. Could you please test with it?

@cvuosalo
Copy link

@mrodozov I don't understand the GeometryDTGeometryBuilderTestDriver error. I tried to test with this PR by using test-prs.sh but did not succeed. Could you please give me simple instructions for how I can test GeometryDTGeometryBuilderTestDriver using this PR?

@mrodozov
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

-1

Failed Tests: UnitTests RelVals
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e91374/12491/summary.html
COMMIT: ebac4e4
CMSSW: CMSSW_11_3_X_2021-01-22-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmsdist/6554/12491/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found errors in the following unit tests:

---> test testDD4hepFilteredView had ERRORS
---> test GeometryDTGeometryBuilderTestDriver had ERRORS

RelVals

  • 11624.91111624.911_TTbar_13+2021_DD4hep+TTbar_13TeV_TuneCUETP8M1_GenSim+Digi+Reco+HARVEST+ALCA/step1_TTbar_13+2021_DD4hep+TTbar_13TeV_TuneCUETP8M1_GenSim+Digi+Reco+HARVEST+ALCA.log

@mrodozov
Copy link
Contributor Author

please test

@mrodozov
Copy link
Contributor Author

want to check with IB later than this PR was merged
cms-sw/cmssw#32721

@cmsbuild
Copy link
Contributor

-1

Failed Tests: UnitTests RelVals
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e91374/12509/summary.html
COMMIT: ebac4e4
CMSSW: CMSSW_11_3_X_2021-01-24-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmsdist/6554/12509/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found errors in the following unit tests:

---> test GeometryDTGeometryBuilderTestDriver had ERRORS

RelVals

  • 11624.91111624.911_TTbar_13+2021_DD4hep+TTbar_13TeV_TuneCUETP8M1_GenSim+Digi+Reco+HARVEST+ALCA/step1_TTbar_13+2021_DD4hep+TTbar_13TeV_TuneCUETP8M1_GenSim+Digi+Reco+HARVEST+ALCA.log

@cmsbuild
Copy link
Contributor

Pull request #6554 was updated.

@mrodozov mrodozov merged commit d014009 into IB/CMSSW_11_3_X/master Jan 25, 2021
@mrodozov mrodozov deleted the update-dd4-newcmake-flag branch January 25, 2021 23:14
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

6 participants