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

Tracker/MTD geometry: clean Geometric(Timing)Det dependence on DDD #30276

Merged
merged 4 commits into from Jun 22, 2020

Conversation

fabiocos
Copy link
Contributor

PR description:

Following the discussion in #28914 , I propose a cleaning of GeometricDet and GeometricTimingDet classes to remove the dependence on DDExpandedNode, which as far as I can see is just fake. In order to make things transparent, I add in DDCMS a few using statements included in DDD which may help in making transparent the migration.

A non trivial dependence remains in GeometricDetExtras and into its timing counterpart. As far as I can see:

  • the use of DDExpandedNode is real, and a fully independent implementation of similar functionalities should be made using DD4hep if needed, so far there is none;

  • this is anyway not impacting the persistent PGeometricDet;

  • the use of this object is for pure debugging purpose only, as far as I can see, so in principle its removal should not break production workflows. For sure this is true for GeometricTimingDetExtras that appears to be a cut-and-paste minimal adjustment of the tracker code, never used and not providing meaningful results at present as far as I can see.

For this reason I propose here a simple removal of this part for MTD, to be possibly replaced with something else in future. But there are already debugging classes that can be possibly extended in case.

As far as Tracker geometry is concerned, it is up to the DPG experts say whether they still need these functionalities, and in case provide a DD4hep-based implementation (that could replace asymptotically the DDD implementation).

PR validation:

MTD geometry unit tests and wf 23234.0 run.

@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-30276/16209

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-30276/16210

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @fabiocos (Fabio Cossutti) for master.

It involves the following packages:

Alignment/OfflineValidation
DetectorDescription/DDCMS
Geometry/MTDNumberingBuilder
Geometry/TrackerNumberingBuilder
RecoTracker/TkDetLayers

@perrotta, @civanch, @Dr15Jones, @makortel, @cvuosalo, @slava77, @christopheralanwest, @ianna, @mdhildreth, @cmsbuild, @tocheng, @tlampen, @pohsun, @kpedro88 can you please review it and eventually sign? Thanks.
@adewit, @makortel, @felicepantaleo, @vargasa, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @gpetruc, @ebrondol, @tocheng, @tlampen, @mschrode, @mmusich, @mtosi, @dgulhan, @slomeo, @venturia this is something you requested to watch as well.
@silviodonato, @dpiparo you are the release manager for this.

cms-bot commands are listed here

@fabiocos
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 17, 2020

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

-1

Tested at: 3660c0b

CMSSW: CMSSW_11_2_X_2020-06-16-2300
SCRAM_ARCH: slc7_amd64_gcc820
You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-eade5a/7149/summary.html

I found follow errors while testing this PR

Failed tests: Build

  • Build:

I found compilation error when building:

>> Compiling  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_2_X_2020-06-16-2300/src/Geometry/MTDNumberingBuilder/src/ES_MTDTopology.cc
>> Compiling  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_2_X_2020-06-16-2300/src/Geometry/MTDNumberingBuilder/src/GeometricTimingDet.cc
>> Compiling  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_2_X_2020-06-16-2300/src/Geometry/MTDNumberingBuilder/src/MTDTopology.cc
>> Compiling  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_2_X_2020-06-16-2300/src/Geometry/MTDNumberingBuilder/src/module.cc
In file included from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_2_X_2020-06-16-2300/src/Geometry/MTDNumberingBuilder/src/module.cc:2:
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_2_X_2020-06-16-2300/poison/Geometry/MTDNumberingBuilder/interface/GeometricTimingDetExtra.h:1:2: error: #error THIS FILE HAS BEEN REMOVED FROM THE PACKAGE.
 #error THIS FILE HAS BEEN REMOVED FROM THE PACKAGE.
  ^~~~~
In file included from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_2_X_2020-06-16-2300/src/Geometry/MTDNumberingBuilder/src/module.cc:4:
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_2_X_2020-06-16-2300/src/Geometry/MTDNumberingBuilder/src/module.cc:7:21: error: 'GeometricTimingDetExtra' was not declared in this scope
 TYPELOOKUP_DATA_REG(GeometricTimingDetExtra);


@cmsbuild
Copy link
Contributor

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

@@ -53,6 +53,8 @@
#include "DataFormats/TrackerCommon/interface/TrackerTopology.h"
#include "Geometry/Records/interface/TrackerTopologyRcd.h"

#include "DetectorDescription/Core/interface/DDCompactView.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please explain this file? It is not migrated. Is it part of GeometricTimingDetExtras?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cvuosalo in the case of this file, as well as the RecoTracker/TkDetLayers/src/Phase2EndcapRingBuilder.cc it is a matter of nested include dependencies, that now are exposed by the removal of the DDExpandedView from GeometricDet. The compilation fails as soon as you update that class.

Copy link
Contributor

Choose a reason for hiding this comment

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

My confusion results from two different versions of this file being included in #29905 and this PR. #29905 has the migrated version. If #29905 is merged first, this PR will probably need to be rebased.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cvuosalo up to the release manager decide which PR is mature for integration first. In case I will rebase mine on that one, not a big issue. BTW, I see that @ianna has already made this addition in #29905

@fabiocos
Copy link
Contributor Author

unhold

@@ -1,5 +1,6 @@
#include "Phase2EndcapRingBuilder.h"
#include "TrackingTools/DetLayers/interface/DetLayerException.h"
#include "FWCore/MessageLogger/interface/MessageLogger.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this file even belong in this PR? It doesn't seem related, and it brings in the requirement for a Reco signature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cvuosalo unfortunately yes, without it the code does not compile, you may easily try yourself.

@cvuosalo
Copy link
Contributor

+1

@cvuosalo
Copy link
Contributor

The decision in this PR is to leave the Tracker version of GeometricDetExtras unmigrated. If the Tracker DPG wants to use GeometricDetExtras tool in the future, they will have to migrate it.

@fabiocos
Copy link
Contributor Author

@cvuosalo as I said, I believe that it is up to the Tracker DPG decide what they want to do with this object. For MTD it is not used so far, and its implementation is not really usable, just a cut-and-paste adjustment of the tracker code that is unusable in its present form. This is not true for the tracker original object, but a DD4hep based implementation of that requires some non completely trivial work, which goes beyond my main task related to MTD. I wonder to which extent this debugging code is really used nowadays, it looks useless to invest work in updating dead code (if that is the case).

@fabiocos
Copy link
Contributor Author

to be even more explicit: this object is used in just 3 places in test modules as far as I can see, either through

https://cmssdt.cern.ch/lxr/source/Geometry/TrackerNumberingBuilder/src/CmsTrackerDebugNavigator.cc#0017

or to print equivalent information, and the name of some higher level volumes in the stack. One should replace the equivalent information with something from DD4hep filtered view. The information coming from DDD logical parts should be appropriately replaced, the copy number is easily accessible, as far as the GeoHistory is concerned, for what I can see most probably the information needed in https://cmssdt.cern.ch/lxr/source/SLHCUpgradeSimulations/Geometry/test/ModuleInfo_Phase2.cc#0433 can be retrieved through the DD4hep geoHistory() as done in https://cmssdt.cern.ch/lxr/source/Geometry/MTDNumberingBuilder/test/DD4hep_MTDTopologyAnalyzer.cc#0210

@perrotta
Copy link
Contributor

+1

  • Just a technical fix for reco
  • Jenkins tests pass

@pohsun
Copy link

pohsun commented Jun 22, 2020

+1

@kpedro88
Copy link
Contributor

+upgrade

@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. @silviodonato, @dpiparo (and backports should be raised in the release meeting by the corresponding L2)

@silviodonato
Copy link
Contributor

+1

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

7 participants