-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Migration to DD4Hep for RPC db Loader #32102
Migration to DD4Hep for RPC db Loader #32102
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32102/19755
|
A new Pull Request was created by @slomeo (Sergio Lo Meo) for master. It involves the following packages: CondTools/Geometry @civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @ggovi can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@cvuosalo : please take a look at https://cmssdt.cern.ch/lxr/search?%21v=CMSSW_11_2_X_2020-11-10-2300&_filestring=&_string=RPCRecoIdealDBLoader Do I have to edit all these .py files adding process.RPCGeometryWriter = cms.EDAnalyzer("RPCRecoIdealDBLoader", instead of process.RPCGeometryWriter = cms.EDAnalyzer("RPCRecoIdealDBLoader") ? |
please test |
The tests are being triggered in jenkins.
|
+1 |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
#include <DetectorDescription/DDCMS/interface/DDFilteredView.h> | ||
#include <DetectorDescription/DDCMS/interface/DDCompactView.h> | ||
#include "DetectorDescription/DDCMS/interface/DDSpecParRegistry.h" | ||
#include "DataFormats/Math/interface/CMSUnits.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is CMSUnits.h
included? I don't see anything from it used in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cvuosalo : I deleted this #include
@@ -4,6 +4,8 @@ | |||
process.load('CondCore.CondDB.CondDB_cfi') | |||
process.load('Configuration.StandardSequences.GeometryExtended_cff') | |||
process.load('Geometry.MuonNumbering.muonNumberingInitialization_cfi') | |||
process.load("Geometry.MuonNumbering.muonGeometryConstants_cff") | |||
process.load('Configuration.StandardSequences.DD4hep_GeometrySim_cff') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is right to include this DD4hep geometry. This file needs to work with both old DD and DD4hep.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cvuosalo : I added the same line for PR #32001 (for CSC). Without it an error appears:
----- Begin Fatal Exception 12-Nov-2020 09:04:49 CET-----------------------
An exception of category 'NoProxyException' occurred while
[0] Processing global begin Run run: 1
[1] Calling method for module RPCRecoIdealDBLoader/'RPCGeometryWriter'
Exception Message:
No data of type "DDCompactView" with label "" in record "IdealGeometryRecord"
Please add an ESSource or ESProducer to your job which can deliver this data.
----- End Fatal Exception -------------------------------------------------
%MSG-i DDCompactViewImpl: 12-Nov-2020 09:04:49 CET EndJob
DDD transient representation has been destructed.
%MSG
} | ||
|
||
const std::vector<double> vtra = {tran.x(), tran.y(), tran.z()}; | ||
const std::vector<double> vtra = {geant_units::operators::convertMmToCm(tran.x()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is worrisome. The old DD code should not need to be changed. Why would the units convention change now for old DD?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cvuosalo : the same code is present in the RPCGeometryBuilder Class. Without it in the Log file I found:
RPCGeometryParsFromDD, tran.x(): 4136.75 tran.y(): 376 tran.z(): -4704.5 // for DD
RPCGeometryParsFromDD, tran.x(): 413.675 tran.y(): 37.6 tran.z(): -470.45 // for DD4Hep
so I think that geant4_units are necessary
We have an open issue on that error #32045 (basically something goes badly wrong during the exit of cmsRun). Do you see any other error messages? Could you try to run with |
@cmsbuild please test |
@makortel , this the recipe:
|
+1 |
Comparison results are now available Comparison Summary:
|
+1 |
@slomeo Thanks, I was able reproduce and got a stack trace in gdb (with
(certainly out of scope of this PR) |
@cvuosalo : while I'm waiting for this PR will be approved and merged I'll update the Ideal Reco DB migration for CSC in order to check if "mm" is the default unit. |
Hi all, do you agree to approve this PR? or do I have to wait till #32045 will be merged? |
@ggovi could you have a look? |
+1 |
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, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
This PR (only for RPC) followed what I did for CSC in #32001 .
PR validation:
Begin processing the 1st record. Run 1, Event 1, LumiSection 1 on stream 0 at 11-Nov-2020 13:26:09.163 CET
MessageLogger Summary
Severity # Occurrences Total Occurrences
dropped waiting message count 0
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
//DD
1, RPCGeometryParsFromDD, detid: 637566989 name: MB1RPC_IGasLeft number of Strips: 90
2, RPCGeometryParsFromDD, dpar.size() == 3, width: 103 length: 58.45 thickness: 0.2
4, RPCGeometryParsFromDD, tran.x(): 413.675 tran.y(): 37.6 tran.z(): -470.45
5, RPCGeometryParsFromDD, x.X(), x.Y(), x.Z(), y.X(), y.Y(), y.Z(), z.X(), z.Y(), z.Z():
5, RPCGeometryParsFromDD 3.06162e-16, -1, 3.12322e-16, -4.78895e-16, -8.5632e-16, 1, -1, 6.40079e-16, 1.15045e-17
//DD4Hep
1, RPCGeometryParsFromDD, detid: 637566989 name: MB1RPC_IGasLeft number of Strips: 90
2, RPCGeometryParsFromDD, dd4hep::Box, width: 103 length: 58.45 thickness: 0.2
4, RPCGeometryParsFromDD, tran.x(): 413.675 tran.y(): 37.6 tran.z(): -470.45
5, RPCGeometryParsFromDD, x.X(), x.Y(), x.Z(), y.X(), y.Y(), y.Z(), z.X(), z.Y(), z.Z():
5, RPCGeometryParsFromDD 2.13888e-16, -1, 2.00422e-16, -9.9934e-17, -4.99491e-16, 1, -1, 3.02876e-16, -1.22527e-16
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
25202.1_TTbar_13+TTbar_13+DIGIUP15APVSimu_PU25+RECOUP15_PU25+HARVESTUP15_PU25 Step0-PASSED Step1-PASSED Step2-PASSED Step3-PASSED - time date Wed Nov 11 13:59:46 2020-date Wed Nov 11 13:32:32 2020; exit: 0 0 0 0
1 1 1 1 tests passed, 0 0 0 0 failed
if this PR is a backport please specify the original PR and why you need to backport that PR:
nothing special