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.

Sign up for GitHub

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

CSC migration to DD4HEP #29111

Merged
merged 4 commits into from
Mar 13, 2020
Merged

CSC migration to DD4HEP #29111

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
005671b
root file deleted
slomeo Mar 5, 2020
4d542dc
fixed problem with parameter useDD4hep, test runTheMatrix ok with wf …
slomeo Mar 6, 2020
513195a
most of the comments have been addressed
slomeo Mar 6, 2020
b91806a
reviewers comments have been addressed
slomeo Mar 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Geometry/CSCGeometryBuilder/BuildFile.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
<use name="Geometry/CSCGeometry"/>
<use name="Geometry/CommonDetUnit"/>
<use name="Geometry/MuonNumbering"/>
<use name="DetectorDescription/DDCMS"/>
<use name="dd4hep"/>
<export>
<lib name="1"/>
</export>
Expand Down
16 changes: 15 additions & 1 deletion Geometry/CSCGeometryBuilder/plugins/BuildFile.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,20 @@
<use name="Geometry/CSCGeometryBuilder"/>
<use name="Geometry/MuonNumbering"/>
<use name="Geometry/Records"/>
<library file="CSCGeometryESModule.cc">

<library name="CSCGeometryPlugins" file="CSCGeometryESModule.cc">
<use name="DetectorDescription/Core"/>
<use name="DetectorDescription/DDCMS"/>
<use name="Geometry/RPCGeometry"/>
<use name="dd4hep"/>
<lib name="Geom"/>
<use name="Geometry/CSCGeometryBuilder"/>
<flags EDM_PLUGIN="1"/>
</library>

<library name="CSCGeometryValidationPlugins" file="CSCGeometryValidate.cc">
<use name="Fireworks/Core"/>
<use name="Geometry/CSCGeometry"/>
<use name="rootgeom"/>
<flags EDM_PLUGIN="1"/>
</library>
31 changes: 29 additions & 2 deletions Geometry/CSCGeometryBuilder/plugins/CSCGeometryESModule.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,14 @@

/*
// \class CSCGeometryESModule
//
// Description: CSC ESModule for DD4hep
//
//
// \author Sergio Lo Meo (sergio.lo.meo@cern.ch) following what Ianna Osburne made for DTs (DD4HEP migration)
// Created: Thu, 05 March 2020
//
// Old DD version author: Tim Cox
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say original author :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

*/
#include "CSCGeometryESModule.h"
#include "Geometry/CSCGeometryBuilder/src/CSCGeometryBuilderFromDDD.h"
#include "Geometry/CSCGeometryBuilder/src/CSCGeometryBuilder.h"
Expand All @@ -11,10 +21,16 @@

#include <memory>

#include "Geometry/MuonNumbering/interface/DD4hep_MuonNumbering.h"
#include "Geometry/MuonNumbering/interface/MuonDDDNumbering.h"
#include "Geometry/MuonNumbering/interface/MuonBaseNumber.h"

using namespace edm;

CSCGeometryESModule::CSCGeometryESModule(const edm::ParameterSet& p)
: alignmentsLabel_(p.getParameter<std::string>("alignmentsLabel")),
: useDDD_(p.getParameter<bool>("useDDD")),
useDD4hep_{p.getUntrackedParameter<bool>("useDD4hep", false)},
Copy link
Contributor

Choose a reason for hiding this comment

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

it has to be tracked, I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Yana, I made the same for GEM migration. It is needed to use Untracked Parameter for dd4hep in order to have no crashes during runTheMatrix tests

Copy link
Contributor

Choose a reason for hiding this comment

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

Strictly speaking tracked vs. untracked should not lead to crashes (or, crashes imply that something else is wrong). Where the failures perhaps complaining a missing parameter? In that case the best solution would be to implement fillDescriptions() to generated and validate the configuration fragment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not a crash, but test failure
(i.e ----- Begin Fatal Exception 05-Mar-2020 18:40:27 CET-----------------------
An exception of category 'Configuration' occurred while
[0] Constructing the EventProcessor
[1] Constructing ESProducer: class=CSCGeometryESModule label=''
Exception Message:
MissingParameter: Parameter 'useDD4hep' not found.
----- End Fatal Exception -------------------------------------------------)

Strictly speaking tracked vs. untracked should not lead to crashes (or, crashes imply >that something else is wrong). Where the failures perhaps complaining a missing >parameter? In that case the best solution would be to implement fillDescriptions() to >generated and validate the configuration fragment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, so the complaint is about missing parameter. The best solutions are to either implement fillDescriptions() (preferred), or propagate the added parameter to all definitions.

Copy link
Contributor

Choose a reason for hiding this comment

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

When you write: "...or propagate the added parameter to all definitions." do you mean to set tUntrackedParameter all the parameters? If it so I think this solution is quite good (and quick)

No, all parameters affecting results/physics should be tracked. I meant to add the parameter in all places where cms.ESProducer("CSCGeometryESModule", ...) is defined. By quick look there does not seem to be that many, but it is in the HLT menu, which would need a customize function in https://github.com/cms-sw/cmssw/blob/master/HLTrigger/Configuration/python/customizeHLTforCMSSW.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found 10 files (see attached picture). In all these .py files do I have to set the dd4hep parameter? Do I have to delete useDD4hep_{p.getUntrackedParameter("useDD4hep", false)} from CSCGeometryESModule.cc? Probably I'll see a compilation error..
Screenshot 2020-03-06 19 32 01

Copy link
Contributor

Choose a reason for hiding this comment

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

In all these .py files do I have to set the dd4hep parameter?

In the first three (under Geometry/CSCGeometry*) yes, the useDD4hep parameter would need to be added.

For the next 6 (under HLTrigger/Configuration) a customize function should be added in
https://github.com/cms-sw/cmssw/blob/master/HLTrigger/Configuration/python/customizeHLTforCMSSW.py
because the HLT menu dumps should not be edited directly.

The last one looks like it can be ignored (it only modifies an existing module, and the file may well be obsolete).

Do I have to delete useDD4hep_{p.getUntrackedParameter("useDD4hep", false)} from CSCGeometryESModule.cc?

It should be changed to useDD4hep_{p.getParameter("useDD4hep")}.

Copy link
Contributor

Choose a reason for hiding this comment

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

please keep in mind that we have a process modifier https://cmssdt.cern.ch/lxr/source/Configuration/ProcessModifiers/python/dd4hep_cff.py that can be used to centralize settings change. This could help in tracking in a transparent way the set of needed modifications

Copy link
Contributor

Choose a reason for hiding this comment

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

@slomeo I think the easiest way is to set the default value by using fillDescriptions(), as suggested by @makortel

alignmentsLabel_(p.getParameter<std::string>("alignmentsLabel")),
myLabel_(p.getParameter<std::string>("appendToDataLabel")) {
auto cc = setWhatProduced(this);

Expand Down Expand Up @@ -52,6 +68,9 @@ CSCGeometryESModule::CSCGeometryESModule(const edm::ParameterSet& p)
if (useDDD_) {
cpvToken_ = cc.consumesFrom<DDCompactView, IdealGeometryRecord>(edm::ESInputTag{});
mdcToken_ = cc.consumesFrom<MuonDDDConstants, MuonNumberingRecord>(edm::ESInputTag{});
} else if (useDD4hep_) {
cpvTokendd4hep_ = cc.consumesFrom<cms::DDCompactView, IdealGeometryRecord>(edm::ESInputTag{});
mdcTokendd4hep_ = cc.consumesFrom<cms::MuonNumbering, MuonNumberingRecord>(edm::ESInputTag{});
} else {
rigToken_ = cc.consumesFrom<RecoIdealGeometry, CSCRecoGeometryRcd>(edm::ESInputTag{});
rdpToken_ = cc.consumesFrom<CSCRecoDigiParameters, CSCRecoDigiParametersRcd>(edm::ESInputTag{});
Expand Down Expand Up @@ -112,6 +131,14 @@ void CSCGeometryESModule::initCSCGeometry_(const MuonGeometryRecord& record, std
CSCGeometryBuilderFromDDD builder;
builder.build(*host, cpv.product(), mdc);
});
} else if (useDD4hep_) {
host->ifRecordChanges<MuonNumberingRecord>(record, [&host, &record, this](auto const& rec) {
host->clear();
edm::ESTransientHandle<cms::DDCompactView> cpv = record.getTransientHandle(cpvTokendd4hep_);
const auto& mdc = rec.get(mdcTokendd4hep_);
CSCGeometryBuilderFromDDD builder;
builder.build(*host, cpv.product(), mdc);
});
} else {
bool recreateGeometry = false;

Expand Down
27 changes: 18 additions & 9 deletions Geometry/CSCGeometryBuilder/plugins/CSCGeometryESModule.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,17 @@
#ifndef CSCGeometryBuilder_CSCGeometryESModule_h
#define CSCGeometryBuilder_CSCGeometryESModule_h

/** \class CSCGeometryESModule
*
* ESProducer for CSCGeometry in MuonGeometryRecord
*
* \author Tim Cox
*/
/*
// \class CSCGeometryESModule
//
// Description: CSC ESModule for DD4hep
//
//
// \author Sergio Lo Meo (sergio.lo.meo@cern.ch) following what Ianna Osburne made for DTs (DD4HEP migration)
// Created: Thu, 05 March 2020
//
// Old DD version author: Tim Cox
*/

#include <FWCore/Framework/interface/ESProducer.h>
#include "FWCore/Framework/interface/ESProductHost.h"
Expand All @@ -16,10 +21,10 @@
#include <Geometry/Records/interface/MuonGeometryRecord.h>
#include <Geometry/CSCGeometry/interface/CSCGeometry.h>
#include "DetectorDescription/Core/interface/DDCompactView.h"

#include <DetectorDescription/DDCMS/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.

please, use ""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

#include "Geometry/Records/interface/IdealGeometryRecord.h"
#include "Geometry/Records/interface/MuonNumberingRecord.h"

#include <Geometry/MuonNumbering/interface/DD4hep_MuonNumbering.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

please, use ""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

#include "Geometry/Records/interface/CSCRecoGeometryRcd.h"
#include "Geometry/Records/interface/CSCRecoDigiParametersRcd.h"
#include "Geometry/MuonNumbering/interface/MuonDDDConstants.h"
Expand Down Expand Up @@ -54,9 +59,12 @@ class CSCGeometryESModule : public edm::ESProducer {
void initCSCGeometry_(const MuonGeometryRecord&, std::shared_ptr<HostType>& host);

edm::ReusableObjectHolder<HostType> holder_;

// DDD
edm::ESGetToken<DDCompactView, IdealGeometryRecord> cpvToken_;
edm::ESGetToken<MuonDDDConstants, MuonNumberingRecord> mdcToken_;
//dd4hep
edm::ESGetToken<cms::DDCompactView, IdealGeometryRecord> cpvTokendd4hep_;
edm::ESGetToken<cms::MuonNumbering, MuonNumberingRecord> mdcTokendd4hep_;

edm::ESGetToken<RecoIdealGeometry, CSCRecoGeometryRcd> rigToken_;
edm::ESGetToken<CSCRecoDigiParameters, CSCRecoDigiParametersRcd> rdpToken_;
Expand All @@ -73,6 +81,7 @@ class CSCGeometryESModule : public edm::ESProducer {
bool debugV;
bool applyAlignment_; // Switch to apply alignment corrections
bool useDDD_; // whether to build from DDD or DB
bool useDD4hep_;
const std::string alignmentsLabel_;
const std::string myLabel_;
};
Expand Down
Loading