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

[CMS] Certain warnings not needed because they report intended behavior #844

Closed
cvuosalo opened this issue Jun 24, 2021 · 13 comments · Fixed by #891
Closed

[CMS] Certain warnings not needed because they report intended behavior #844

cvuosalo opened this issue Jun 24, 2021 · 13 comments · Fixed by #891

Comments

@cvuosalo
Copy link

As used by CMS, DD4hep issues these warnings and info messages:

Evaluator        WARN  +++ Overwriting variable: name=world_x value=101*m  Evaluator::Object : existing variable [setVariable error]
Evaluator        WARN  +++ Overwriting variable: name=world_y value=101*m  Evaluator::Object : existing variable [setVariable error]
Evaluator        WARN  +++ Overwriting variable: name=world_z value=450*m  Evaluator::Object : existing variable [setVariable error]
DD4hep           WARN  ++ Using globally Geant4 unit system (mm,ns,MeV)

PlacedVolume     INFO  REFLECTION: (x.Cross(y)).Dot(z):       -1 Parent: eregalgo:EFAW [TGeoVolume] Daughter: eregalgo:EHAWR [TGeoVolumeAssembly]

These messages reported intended behavior. The world variables need to be overwritten for use by CMS, CMS wants to use the Geant4 unit system, and the EFAW assembly should be reflected. In these cases, only a debug message would be needed, not a warning or info. Of course, in other cases, warnings should still appear, just not for intended behavior.

See here for the source of the overwriting warning:

os << prefix << "existing variable";

As CMS moves to production use of DD4hep, it would be nice to remove unnecessary warnings from our logs.

These messages have been appearing for a long time and are still present with version 1.17.0.

@andresailer
Copy link
Member

Are the world_ values coming from here:
https://github.com/cms-sw/cmssw/blob/7535949dd24341cba7d053bf7e39633a5dea4725/DetectorDescription/DDCMS/plugins/dd4hep/DDDefinitions2Objects.cc#L2059-L2061
?

Which are the XML files used? Just to be sure there isn't any overwriting done, and one of the placed could be removed.

@andresailer
Copy link
Member

And I can see that you wrote this isn't in any CMS xml files in the issue you linked.
I'll check if we see these warnings in the CLIC detector, I haven't spotted defaults in dd4hep yet.

@andresailer
Copy link
Member

For the CLIC detector were we set the world_x/y/z in the XML I don't get warnings for the world
I can see

Evaluator        WARN  +++ Overwriting variable: name=HCal_cell_size value=3.0*cm  Evaluator::Object : existing variable [setVariable error]
Detector         WARN  +++ Object 'HCal_cell_size' is already defined. New value will be ignored

that second warning is what I remember adding and mentioned in the meeting. (it doesn't matter for us because we use the same value in both cases, but easer to have it twice

@ianna
Copy link
Contributor

ianna commented Jun 24, 2021

@andresailer - we do not define these constants in XML. Does it mean that static long load_dddefinition(Detector& det, xml_h element) is called after the World has been created?

@andresailer
Copy link
Member

Is the full detector construction log somewhere visible?
What is the top level XML you are using?

@MarkusFrankATcernch
Copy link
Contributor

@ianna

  1. For you guys (and actually on your request) the possibility was put in place to create the world volume before calling Detector::init() to allow for a dedicated shape of the world volume different from a box, which is sufficient for the rest of the world.

  2. DD4hep does not define these variables itself. It requires them in Detector::init() if the top volume is NOT set to DD4hep's TGeoManager upfront. This should not be the case for you, since you define the world volume upfront.

  3. as a corollary of 2): Somewhere in your XML files you must have 2 definitions of the world coordinates world_x, world_y and world_z must be present in some definition section. You have to attach a debugger ans set a break point. then you will know immediately....

@ianna
Copy link
Contributor

ianna commented Jun 25, 2021

@ianna

  1. For you guys (and actually on your request) the possibility was put in place to create the world volume before calling Detector::init() to allow for a dedicated shape of the world volume different from a box, which is sufficient for the rest of the world.

This warning comes before we parse out xml description where we would have another definition of a non-box volume that we'd like to be the World one. We cannot define it in the dd4hep configuration xml because it is too complex, so we try to make the World box be big enough to hold this other volume.

  1. DD4hep does not define these variables itself. It requires them in Detector::init() if the top volume is NOT set to DD4hep's TGeoManager upfront. This should not be the case for you, since you define the world volume upfront.

I think, the static long load_dddefinition(Detector& det, xml_h element) is our entry point. How can we do anything beforehand?

  1. as a corollary of 2): Somewhere in your XML files you must have 2 definitions of the world coordinates world_x, world_y and world_z must be present in some definition section. You have to attach a debugger ans set a break point. then you will know immediately....

Do you mean we must define then in xml? Or we must have defined them in xml?

@MarkusFrankATcernch
Copy link
Contributor

@ianna
Between creating your instance of DD4hep::Detector and calling the plugin, which in turn calls load_dddefintiions() something must be executed, which causes the described behavior. This can be parsing some xml, this can be adding some hand-crafted constants in code or it could be loading some dd4hep file in ROOT format containing these constants: I cannot tell you exactly what, but you need to analyze with the debugger what happens at which time. Otherwise you rest with seeking the needle in the hay-stack.

@ianna
Copy link
Contributor

ianna commented Jun 25, 2021

@ianna
Between creating your instance of DD4hep::Detector and calling the plugin, which in turn calls load_dddefintiions() something must be executed, which causes the described behavior. This can be parsing some xml, this can be adding some hand-crafted constants in code or it could be loading some dd4hep file in ROOT format containing these constants: I cannot tell you exactly what, but you need to analyze with the debugger what happens at which time. Otherwise you rest with seeking the needle in the hay-stack.

thanks @MarkusFrankATcernch , I'll need to do some debugging

@MarkusFrankATcernch
Copy link
Contributor

MarkusFrankATcernch commented Jun 25, 2021

@ianna
The evaluator is shared between all instances of DD4hep::Detector.
I remember you also requested named instances to have multiples at a time.
If you use multiple instances, the evaluator is shared between them.
Please keep this in mind if you encounter a surprise.

@cvuosalo
Copy link
Author

cvuosalo commented Oct 6, 2021

@MarkusFrankATcernch CMS creates two instances of a DD4hep::Detector and defines a few of the same constants for both, which creates the "overwriting" message. To avoid this, I would like to check whether a constant is already defined before calling dd4hep::_toDefinition(). I found dd4hep::_getEnviron(const string&) that could be used to check for defined constants, but it doesn't appear in any public header file. Can you suggest a way to check whether a constant has already been defined?

@cvuosalo
Copy link
Author

cvuosalo commented Oct 8, 2021

I submitted a PR to prevent the "Overwriting variable" messages when DD4hep is called in CMSSW: cms-sw/cmssw#35594.

@cvuosalo
Copy link
Author

There are still two unnecessary messages with DD4hep v01-19:

DD4hep           WARN  ++ Using globally Geant4 unit system (mm,ns,MeV)
PlacedVolume     INFO  REFLECTION: (x.Cross(y)).Dot(z):       -1 Parent: eregalgo:EFAW [TGeoVolume] Daughter: eregalgo:EHAWR [TGeoVolumeAssembly]

The unit system message should be INFO or DEBUG, while the reflection message should be DEBUG.

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

Successfully merging a pull request may close this issue.

4 participants