Phase2-hgx108 Fix a bug for numbering HGCal #22968
Conversation
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22968/4352 |
|
A new Pull Request was created by @bsunanda for master. It involves the following packages: Geometry/HGCalCommonData @civanch, @Dr15Jones, @ianna, @mdhildreth, @cmsbuild, @kpedro88 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild Please test |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
| @@ -109,6 +105,12 @@ std::pair<int,int> HGCalDDDConstants::assignCellHexagon(float x, | |||
| double xx(x), yy(y); | |||
| //First the wafer | |||
| int wafer = cellHex(xx, yy, rmax_, hgpar_->waferPosX_, hgpar_->waferPosY_); | |||
| if (wafer < 0 || wafer >= (int)(hgpar_->waferTypeT_.size())) | |||
| edm::LogWarning("HGCalGeom") << "Wafer " << wafer << ":" | |||
There was a problem hiding this comment.
maybe this warning message could be more instructive instead of just saying error?
| @@ -132,6 +134,12 @@ HGCalParameters::hgtrap HGCalDDDConstants::getModule(unsigned int indx, | |||
|
|
|||
| HGCalParameters::hgtrap mytr; | |||
| if (hexType) { | |||
| if (indx >= hgpar_->waferTypeL_.size()) | |||
| edm::LogWarning("HGCalGeom") << "Index " << indx << ":" | |||
| #endif | ||
| if (ok) { | ||
| if (moditr->second >= 0) { | ||
| if (mod >= (int)(hgpar_->waferTypeT_.size())) | ||
| edm::LogWarning("HGCalGeom") << "Module " << mod << ":" |
| #endif | ||
| return ok; | ||
| } | ||
|
|
||
| bool HGCalDDDConstants::isValidCell(int lay, int wafer, int cell) const { | ||
|
|
||
| if (wafer >= (int)(hgpar_->waferPosX_.size())) | ||
| edm::LogWarning("HGCalGeom") << "Valid Wafer " << wafer << ":" |
| @@ -389,6 +411,12 @@ std::pair<int,int> HGCalDDDConstants::simToReco(int cell, int lay, int mod, | |||
| std::pair<int,float> index = getIndex(lay, false); | |||
| int i = index.first; | |||
| if (i < 0) { | |||
| edm::LogWarning("HGCalGeom") << "Wrong Laye # " << lay; | |||
|
other comments:
does the actual geometry need to be revised? |
|
@kpedro88 I make the bug fix which will not create bad DetId's. I am not going to change this geometry right now. I shall make the warnings a bit more explicit. |
|
Comparison is ready Comparison Summary:
|
|
+1 |
|
+1 |
|
+1 |
cmsbuild
added
fully-signed
simulation-approved
and removed
pending-signatures
simulation-pending
labels
|
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. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
|
Now phase2 workflow(s) are full of The example is from premixing stage1 workflow because at the moment it is the only finished phase2 workflow for CMSSW_10_2_X_2018-04-19-2300. The printouts could be specific to premixing, but my first though is that the customizations on top of standard DIGI+classical pileup mixing should not cause these. Something needs to be fixed in the downstream code I presume? |
|
There was some bug in earlier SIM step. All we can do now is to suppress the print statement. Also may introduce check to ignore those hits coming in the mixing step.
…________________________________
From: Matti Kortelainen [notifications@github.com]
Sent: 20 April 2018 14:18
To: cms-sw/cmssw
Cc: Sunanda Banerjee; Mention
Subject: Re: [cms-sw/cmssw] Phase2-hgx108 Fix a bug for numbering HGCal (#22968)
Now phase2 workflow(s) are full of
%MSG-w HGCalGeom: MixingModule:mix 20-Apr-2018 07:57:46 CEST Run: 1 Event: 2
Invalid Wafer # 669should be < 669 ***** ERROR *****
%MSG
%MSG-w HGCalGeom: MixingModule:mix 20-Apr-2018 07:57:46 CEST Run: 1 Event: 2
Invalid Wafer # 669should be < 669 ***** ERROR *****
%MSG
https://cmssdt.cern.ch/SDT/cgi-bin/buildlogs/slc6_amd64_gcc630/CMSSW_10_2_X_2018-04-19-2300/pyRelValMatrixLogs/run/20261.98_NuGun+SingleNuE10_cf_2023D17PU_PremixHLBeamSpotFullPU_Premix/step1_NuGun+SingleNuE10_cf_2023D17PU_PremixHLBeamSpotFullPU_Premix.log
The example is from premixing stage1 workflow because at the moment it is the only finished phase2 workflow for CMSSW_10_2_X_2018-04-19-2300. The printouts could be specific to premixing, but my first though is that the customizations on top of standard DIGI+classical pileup mixing should not cause these.
Something needs to be fixed in the downstream code I presume?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#22968 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AEzMujDFSgeOuDgiJf8bgMN1q7HuTtQDks5tqdIDgaJpZM4TVibb>.
|
|
We should really update the minbias input sample. Maybe with pre2 relvals? |
|
Ok, so the printouts are specific to the pileup GEN-SIM? And I see @kpedro88 was faster than me to suggest updating the minbias input. In this case I'm personally fine with keeping the printouts as the premixing stage1 is the only affected workflow of standard matrix, and hopefully we'd get the new GEN-SIM rather soon. |
Let's inform PdmV @fabozzi @prebello @GurpreetSinghChahal then that we'd ask 2023D17 MinBias GEN-SIM to be produced with 10_2_0_pre2 and subsequently the corresponding dataset template to updated in |
|
@jnsandhya : please note this request of updating Phase2 GENSIM in next 10_1_0_pre2 release |
|
This has now been submitted as a new PR here: |
|
The warning messages #22968 (comment) are indeed gone with the 10_1_0_pre2 GEN-SIM for pileup. |
|
Just to avoid confusion, the string is updated with 10_2_0_pre2. |
It used to access elements beyond boundary in coverting sim to reco detId