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

Mix use of DD4hep and DD4Hep across CMSSW. #33373

Closed
srimanob opened this issue Apr 8, 2021 · 19 comments
Closed

Mix use of DD4hep and DD4Hep across CMSSW. #33373

srimanob opened this issue Apr 8, 2021 · 19 comments

Comments

@srimanob
Copy link
Contributor

srimanob commented Apr 8, 2021

Seem there are a mixing of "DD4hep" and "DD4Hep" across CMSSW code. It looks like "DD4hep" is the right one to use.

Examples of "DD4Hep":
Geometry/DTGeometryBuilder/src/DTGeometryBuilderFromDD4Hep.cc
Geometry/DTGeometryBuilder/src/DTGeometryBuilderFromDD4Hep.h
MagneticField/Engine/test/regression.py
MagneticField/Engine/test/runTest.sh
SimG4Core/PrintGeomInfo/test/SimFileCompare.cpp
Geometry/RPCGeometryBuilder/src/RPCGeometryParsFromDD.cc
Geometry/EcalCommonData/plugins/EcalSimParametersESModule.cc
...

It does not affect anything, just something not the same across CMSSW. Should I try to migrate all of them to be the same?

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 8, 2021

A new Issue was created by @srimanob Phat Srimanobhas.

@Dr15Jones, @dpiparo, @silviodonato, @smuzaffar, @makortel, @qliphy can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

@makortel
Copy link
Contributor

makortel commented Apr 8, 2021

assign geometry, simulation, reconstruction

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 8, 2021

New categories assigned: geometry,reconstruction,simulation

@Dr15Jones,@cvuosalo,@mdhildreth,@mdhildreth,@slava77,@perrotta,@makortel,@jpata,@ianna,@civanch,@civanch you have been requested to review this Pull request/Issue and eventually sign? Thanks

@civanch
Copy link
Contributor

civanch commented Oct 29, 2021

Is this issue resolved or not yet?

@cvuosalo
Copy link
Contributor

cvuosalo commented Nov 2, 2021

There are about 100 files with DD4Hep instead of DD4hep. It would be a fair amount of work to change them all while being sure not to make any mistakes or typos. I'm not sure it is worth the effort.

@civanch
Copy link
Contributor

civanch commented Nov 3, 2021

@cvuosalo , how many files start from DD4hep? If more than 10-20 then we cannot fix this.

@cvuosalo
Copy link
Contributor

cvuosalo commented Nov 4, 2021

@civanch The capitalization of DD4hep is standard. It is in hundreds of files. We don't want to change it.

@ianna
Copy link
Contributor

ianna commented Nov 5, 2021

@cvuosalo - IMHO, it would be nice to have identical spelling in all Python configurations. https://cmssdt.cern.ch/lxr/search?%21v=CMSSW_12_2_X_2021-11-04-2300&_filestring=py&_string=fromDD4Hep

It is odd that two geometry producers use different syntax in the same fragment:
https://cmssdt.cern.ch/lxr/source/Alignment/MuonAlignment/test/convertSQLitetoXML_cfg.py#0028

@srimanob
Copy link
Contributor Author

srimanob commented Nov 5, 2021

Hi @ianna @cvuosalo @civanch

Thanks for following up. I agree with @ianna, at least the configuration should be the same. I propose to keep this issue open. I can help with fixing PR.

@civanch
Copy link
Contributor

civanch commented Nov 5, 2021

I agree. Also it is possible adiabatically change the situation:

  1. better to have correct names in plugins
  2. If a class is used only inside its sub-package it is quite easy make a change, when a this sub-package is changed for another reason.

@ianna
Copy link
Contributor

ianna commented Nov 5, 2021

this should be fixed in #36004 and #36003

@srimanob
Copy link
Contributor Author

srimanob commented Nov 5, 2021

Thanks very much @ianna

@ianna
Copy link
Contributor

ianna commented Nov 6, 2021

this should be fixed in #36004 and #36003

and the final cleanup is done in #36021

@ianna
Copy link
Contributor

ianna commented Nov 12, 2021

@srimanob - please, check if the issue can be closed. Thanks!

@srimanob
Copy link
Contributor Author

Thanks @ianna I think this issue can be closed.
@cms-sw/geometry-l2 @cms-sw/simulation-l2 @cms-sw/reconstruction-l2 Could you please consider, and sign? Or please comment if something else remains.

@cvuosalo
Copy link
Contributor

+1

@civanch
Copy link
Contributor

civanch commented Nov 28, 2021

+1

@jpata
Copy link
Contributor

jpata commented May 17, 2022

+reconstruction
-git grep DD4Hep gives only one hit, in Validation/HGCalValidation

@cmsbuild
Copy link
Contributor

This issue is fully signed and ready to be closed.

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

No branches or pull requests

8 participants