Prepare first release using Geant4 units (mm,ns,MeV) #772

MarkusFrankATcernch · 2021-01-06T23:26:52Z

BEGINRELEASENOTES

This is the first release prepared to use Geant4 units (mm,ns,MeV). This compilation mode can be steered by a cmake flag: -DDD4HEP_USE_GEANT4_UNITS=ON. Once set, the flag is automatically propagated to depending projects in the generated DD4hepConfig.cmake. It programs TGeo to use the Geant4 units system. The unit system is applied also to depending quantities like interaction lengths etc.
Update some tests and ensure compatibility in the checks/reference files for both unit system. There is one caveat: Mesh-creation of shapes depends to some degree on the unit system, because mesh points are removed depending on a tolerance value which is the same for both systems.
A fix had to be applied to propagate the flag DD4HEP_BUILD_DEBUG to depending projects. This flag affects some object layouts and hence MUST be applied also to depending projects.
Remove inclusions of Plugins.h from being processed by rootcling. Plugins.h uses #include <any>, which cannot be processed.
ENDRELEASENOTES

MarkusFrankATcernch · 2021-01-06T23:36:08Z

@andresailer Could you please have a look to the changes in cmake ? This is not really my strength....

andresailer

I don't understand why the manual propagation of DD4HEP_DEBUG is necessary, unless you have something that doesn't link (transitively) against DDCore?

DDCore/src/Volumes.cpp

andresailer · 2021-01-07T08:40:48Z

DDCore/src/plugins/ShapePlugins.cpp

@@ -950,8 +957,8 @@ void* shape_mesh_verifier(Detector& description, int argc, char** argv)    {
        solid.setDimensions(params);
      }
      else if ( isInstance<PseudoTrap>(solid) )   {
-        auto params = solid.dimensions();
-        solid.setDimensions(params);
+        //auto params = solid.dimensions();


Why this change?

Ohh. This was a mistake.

andresailer · 2021-01-07T08:43:08Z

cmake/DD4hepBuild.cmake

@@ -565,6 +570,10 @@ function(dd4hep_add_dictionary dictionary )
    LIST(APPEND comp_defs ${def})
  endforeach()

+if(DD4HEP_BUILD_DEBUG)


Suggested change

if(DD4HEP_BUILD_DEBUG)

if(DD4HEP_BUILD_DEBUG)

But is this really necessary?

Apparently yes. ROOT data streaming simply crashed for conditions, where this flag actually changes the structure layout.
Also the TStreamerInfo was wrong.
For this reason the compile flag must be passed to rootcling.

The idea is that rootcling is being passed this information from one of the targets (or USE parameter?), if you can tell me more details I will investigate.

I do not know. The very best would probably be to generate some "config.h", which contains all relevant compile parameters.
This would ensure that such parameters, which cannot be changed for one release build are also properly propagated
to all client projects....

Seems to me this

DD4hep/DDCore/CMakeLists.txt

Line 43 in c179ef4

USES DDParsers ${XML_LIBRARIES} ${TBB_IMPORTED_TARGETS}

is missing DDCore and thus not inheriting the DD4HEP_DEBUG.
I did get crashes for various tests without DDCore here when DD4HEP_BUILD_DEBUG was ON

And probably also in the other dictionaries in DDCore/CMakeLists.txt

Yes. These are exactly the dictionaries in question.
But how can DDCore depend on DDCore ?

So your claim is:
If the flags are set in DD4hep explicitly, the statement:

IF(DD4HEP_BUILD_DEBUG MATCHES "ON" OR (CMAKE_BUILD_TYPE MATCHES "DEBUG|Debug" AND NOT DD4HEP_BUILD_DEBUG MATCHES "OFF"))
target_compile_definitions(DDCore PUBLIC DD4HEP_DEBUG=1)
ENDIF()

Would do all the job ?
However, it then must move to cmake/DD4hepBuild.cmake so that it is picked up by all clients.

The properties are stored in DD4hepConfig-targets.cmake there is no need to move this anywhere else

# Create imported target DD4hep::DDCore add_library(DD4hep::DDCore SHARED IMPORTED) set_target_properties(DD4hep::DDCore PROPERTIES INTERFACE_COMPILE_DEFINITIONS "DD4HEP_DEBUG=1" INTERFACE_COMPILE_FEATURES "cxx_std_14" INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include" INTERFACE_LINK_LIBRARIES "DD4hep::DDParsers;ROOT::Core;ROOT::Rint;ROOT::Tree;ROOT::Physics;ROOT::Geom;ROOT::GenVector;XercesC::XercesC;dl" )

But how can DDCore depend on DDCore ?

Thanks to the generator expressions which are evaluated at build time

DD4hep/cmake/DD4hepBuild.cmake

Line 572 in c179ef4

LIST(APPEND comp_defs $<TARGET_PROPERTY:${DEP},INTERFACE_COMPILE_DEFINITIONS>)

andresailer · 2021-01-07T08:49:01Z

CMakeLists.txt

+  IF(DD4HEP_BUILD_DEBUG MATCHES "ON" OR (CMAKE_BUILD_TYPE MATCHES "DEBUG|Debug" AND NOT DD4HEP_BUILD_DEBUG MATCHES "OFF"))
+    message(STATUS "BUILD DD4HEP with debug extensions")
+    set(DD4HEP_BUILD_DEBUG ON)
+    add_compile_definitions(DD4HEP_DEBUG=1)


Why is this needed? The definition for the DDCore target should be inherited by anything that links against DDCore, which should be everything, no? (This should also propagate to any project using DDCore outside of DD4hep)

Hmm. Must retest this.

andresailer · 2021-01-07T08:56:48Z

cmake/DD4hepConfig.cmake.in

@@ -50,6 +52,10 @@ endif()
 FIND_DEPENDENCY(Boost REQUIRED)
 DD4HEP_SETUP_BOOST_TARGETS()

+if(DD4HEP_BUILD_DEBUG)
+  add_compile_definitions(DD4HEP_DEBUG=1)


YES. Absolutely.
Everything depending on DDCore (better including DD4hep/detail/ConditionsInterna.h) needs this flag.
Otherwise you have conflicting object layouts.

andresailer · 2021-01-07T13:55:44Z

cmake/DD4hepBuild.cmake

@@ -43,6 +43,11 @@ macro(dd4hep_set_compiler_flags)
    set ( COMPILER_FLAGS ${COMPILER_FLAGS} -Winconsistent-missing-override -Wno-c++1z-extensions -Wheader-hygiene )
  endif()

+  IF( DD4HEP_USE_GEANT4_UNITS )
+    MESSAGE(STATUS "+++ Building DD4hep with Geant4 units......")
+    add_compile_definitions(DD4HEP_USE_GEANT4_UNITS=1)


This should just be added as a target_compile_definitions to DDG4(?)

Maybe.
Then it also must enter in cmake/DD4hepConfig.cmake.in because DD4hepUnits.h must be consistently used for all
depending code.

Which it would because of the transitivity of the cmake target properties.

I think I misunderstood your comment.
The flag DD4HEP_USE_GEANT4_UNITS has nothing to do with Geant4.
It tells dd4hep to instruct TGeo to natively compute all quantities in the same units as Geant4 has (mm,ns,MeV).
This is independent of DDG4. I will try to do the same thing as with DD4HEP_DEBUG

…nits

…sults in both unit schemes.

…comments

MarkusFrankATcernch requested a review from andresailer January 6, 2021 23:35

andresailer reviewed Jan 7, 2021

View reviewed changes

MarkusFrankATcernch added 6 commits January 8, 2021 15:38

Prepare for using DD4hep with Geant4 units (mm,ns,MeV)

51ffe81

Fix DDCore dictionaries to not include Plugins.h

320893b

Propagate DD4HEP_DEBUG definition properly to depending compilation u…

80ea81e

…nits

Fix not working examples. Provide PseudoTrap tests with equal mesh re…

d5451d3

…sults in both unit schemes.

Fix not working examples. Provide PseudoTrap tests with equal mesh re…

e77ebbd

…sults in both unit schemes.

Use compile flag propagation as suggested by Andre. Fix according to …

bf7d17f

…comments

MarkusFrankATcernch force-pushed the master branch from 2c304f4 to bf7d17f Compare January 8, 2021 15:31

MarkusFrankATcernch merged commit 46fff9d into AIDASoft:master Jan 8, 2021

MarkusFrankATcernch mentioned this pull request Jan 14, 2021

CMS needs DD4hep version that uses Geant4 units convention #770

Closed

petricm added the enhancement label Feb 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prepare first release using Geant4 units (mm,ns,MeV) #772

Prepare first release using Geant4 units (mm,ns,MeV) #772

MarkusFrankATcernch commented Jan 6, 2021 •

edited by petricm

Loading

MarkusFrankATcernch commented Jan 6, 2021

andresailer left a comment

andresailer Jan 7, 2021

MarkusFrankATcernch Jan 7, 2021

andresailer Jan 7, 2021

MarkusFrankATcernch Jan 7, 2021

andresailer Jan 7, 2021

MarkusFrankATcernch Jan 7, 2021

andresailer Jan 7, 2021

andresailer Jan 7, 2021

MarkusFrankATcernch Jan 7, 2021

andresailer Jan 7, 2021

andresailer Jan 7, 2021

MarkusFrankATcernch Jan 7, 2021

andresailer Jan 7, 2021

MarkusFrankATcernch Jan 7, 2021

andresailer Jan 7, 2021

MarkusFrankATcernch Jan 7, 2021

andresailer Jan 7, 2021

MarkusFrankATcernch Jan 8, 2021

Prepare first release using Geant4 units (mm,ns,MeV) #772

Prepare first release using Geant4 units (mm,ns,MeV) #772

Conversation

MarkusFrankATcernch commented Jan 6, 2021 • edited by petricm Loading

MarkusFrankATcernch commented Jan 6, 2021

andresailer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkusFrankATcernch commented Jan 6, 2021 •

edited by petricm

Loading