bsunanda:Phase2-hgx67 Correct reconstruction geometry for EE and FH with full coverage #16524

bsunanda · 2016-11-08T20:07:00Z

The test functions work ok for reconstruction geometry. Some of the validation codes are updated and C3 for calorimeter phase2 is defined

cmsbuild · 2016-11-08T20:07:18Z

A new Pull Request was created by @bsunanda for CMSSW_8_1_X.

It involves the following packages:

Configuration/Geometry
Geometry/CaloTopology
Geometry/HGCalCommonData
Geometry/HGCalGeometry
Validation/HGCalValidation

@civanch, @Dr15Jones, @ianna, @mdhildreth, @dmitrijus, @cmsbuild, @vanbesien, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @cseez, @vandreev11, @sethzenz, @kpedro88, @lgray, @Martin-Grunewald, @pfs this is something you requested to watch as well.
@slava77, @smuzaffar you are the release manager for this.

cms-bot commands are listed here #13028

bsunanda · 2016-11-08T20:07:23Z

@cmsbuild Please test

cmsbuild · 2016-11-08T20:07:35Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/16265/console

cmsbuild · 2016-11-08T21:56:10Z

-1

Tested at: d3d8edb

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16524/16265/summary.html

I found follow errors while testing this PR

Failed tests: UnitTests

Unit Tests:

I found errors in the following unit tests:

---> test runtestRecoLocalCaloHGCalRecProducers had ERRORS

cmsbuild · 2016-11-08T21:56:12Z

Comparison job queued.

cmsbuild · 2016-11-08T22:30:18Z

Pull request #16524 was updated. @civanch, @Dr15Jones, @cvuosalo, @ianna, @mdhildreth, @dmitrijus, @cmsbuild, @slava77, @vanbesien, @davidlange6 can you please check and sign again.

bsunanda · 2016-11-08T22:40:17Z

@cmsbuild Please test

cmsbuild · 2016-11-08T22:46:26Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16524/16265/summary.html

cmsbuild · 2016-11-08T22:46:40Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/16270/console

cmsbuild · 2016-11-09T01:16:56Z

+1
Tested at: b0da39f
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16524/16270/summary.html

cmsbuild · 2016-11-09T01:16:59Z

Comparison job queued.

bsunanda · 2016-11-10T22:55:53Z

@slava77 @dmitrijus Please approve this PR

ianna · 2016-11-11T10:20:59Z

@smuzaffar - FYI, this PR should fix failing unit test

smuzaffar · 2016-11-11T10:30:43Z

Thanks @ianna . Indeed the unit test now run fine.

bsunanda · 2016-11-14T06:21:25Z

@dmitrijus Please approve this

slava77 · 2016-11-14T19:20:47Z

@bsunanda
modifications in RecoLocalCalo/HGCalRecProducers/test/testHGCalRecoLocal_cfg.py appear to be not necessary.

This is the reason for lack of my signature.

bsunanda · 2016-11-15T00:22:06Z

@slava77 It may not be necessary to avoid the unit test failure but strictly speaking the choice of the eras is the correct one in the new commit

bsunanda · 2016-11-15T00:23:36Z

@slava77 @davidlange6 This will most likely become the TDR geometry of HGCal. So it is rather important that this one and the following PR to become a part of 8_11_0

dmitrijus · 2016-11-15T12:52:32Z

+1

slava77 · 2016-11-15T17:09:30Z

+1

for #16524 b0da39f

reco is minimally affected: only in RecoLocalCalo/HGCalRecProducers/test/testHGCalRecoLocal_cfg.py where either choice of era doesn't seem to matter
jenkins tests pass and comparisons with baseline show difference only in 2023 HGCAL workflows. A cursory check of O(10) plots in HGC hits, clusters, PF and jets appears to show roughly similar distributions (nothing is obviously broken)

cmsbuild · 2016-11-15T17:09:46Z

This pull request is fully signed and it will be integrated in one of the next CMSSW_8_1_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @smuzaffar

slava77 · 2016-11-15T22:41:33Z

Configuration/Geometry/scripts/dict2023Geometry.py

@@ -353,6 +353,78 @@
            'from Geometry.EcalMapping.EcalMappingRecord_cfi import *',
        ],
        "era" : "run2_HE_2017, run2_HF_2017, run2_HCAL_2017, run3_HB, phase2_hcal, phase2_hgcal",
+    },
+    "C3" : {


@kpedro88
shouldn't edits in this file be accompanied by more changes?
(e.g. README, generated geometry cff files and definition of new workflows)

@slava77 it's acceptable if @bsunanda wants to add the necessary geometry info for C3 without making a workflow for it right away. (So in general, adding items is okay, but editing anything that's currently used in a workflow should be accompanied by rerunning the script to update any affected cff files.)

But yes, I would appreciate keeping the README up to date, i.e. C3 corresponds to HGCal v8 + Phase2 HCAL.

At this moment it is a bit premature to add a new or modify existing workflow. I shall update the README - but I shall do that in a separate PR (this one is signed by all and updating this PR will require it go through the entire cycle all over again)

The PR is now provided to add the comment in README file

kpedro88 · 2016-11-17T16:11:27Z

@davidlange6 can this be merged?

davidlange6 · 2016-11-17T16:13:35Z

did @lgray answer the concern raised by @slava77 in the orp (and in this thread)?

On Nov 17, 2016, at 5:11 PM, Kevin Pedro notifications@github.com wrote:

@davidlange6 can this be merged?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

slava77 · 2016-11-17T16:14:40Z

On 11/17/16 8:13 AM, David Lange wrote:

did @lgray answer the concern raised by @slava77 in the orp (and in this
thread)?

yes, that's why I signed the 81X version earlier on Tue.

On Nov 17, 2016, at 5:11 PM, Kevin Pedro notifications@github.com wrote:

@davidlange6 can this be merged?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#16524 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/AEdcbi_-wTcNDBn97SyT34WwxnUVFsqYks5q_H0vgaJpZM4Ks2iH.

davidlange6 · 2016-11-17T18:03:19Z

+1
ok, missed the answer.

slava77 · 2016-11-18T17:22:35Z

now that this PR lead to broken phase2 workflows, I guess it should be reverted
both in 81X and 90X

kpedro88 · 2016-11-19T05:13:05Z

@bsunanda here is the backtrace of the exception:

#3  0x00007fffd279a785 in std::unordered_map<short, short, std::hash<short>, std::equal_to<short>, std::allocator<std::pair<short const, short> > >::at (
    __k=@0x7fffffff05ec: 565, this=0x7fffe8389f58)
    at /cvmfs/cms-ib.cern.ch/2016-47/slc6_amd64_gcc530/external/gcc/5.3.0/include/c++/5.3.0/bits/unordered_map.h:689
#4  HGCalTriggerGeometryHexImp2::getModuleFromCell (this=0x7fffe8389e00, cell_id=<optimized out>)
    at /uscms_data/d3/pedrok/phase2/wfs/CMSSW_9_0_X_2016-11-18-2300/src/L1Trigger/L1THGCal/plugins/geometries/HGCalTriggerGeometryHexImp2.cc:96
#5  0x00007fffd2823d3b in HGCalTriggerDigiProducer::produce (this=0x7fffd2bec800, e=..., es=...)
    at /uscms_data/d3/pedrok/phase2/wfs/CMSSW_9_0_X_2016-11-18-2300/src/L1Trigger/L1THGCal/plugins/HGCalTriggerDigiProducer.cc:103

print cell_det_id
$4 = {<DetId> = {static kDetOffset = 28, static kSubdetOffset = 25, id_ = 1741305135}, static kHGCalCellOffset = 0, static kHGCalCellMask = 255, 
  static kHGCalWaferOffset = 8, static kHGCalWaferMask = 1023, static kHGCalWaferTypeOffset = 18, static kHGCalWaferTypeMask = 1, 
  static kHGCalLayerOffset = 19, static kHGCalLayerMask = 31, static kHGCalZsideOffset = 24, static kHGCalZsideMask = 1, static kHGCalMaskCell = -256, 
  static Undefined = <optimized out>}

The wafer of this DetId is 565 ((1741305135>>8)&1023), but the map ends at 563:

print wafer_to_module_ee_
$5 = std::unordered_map with 564 elements = {[563] = 265, [562] = 264, [561] = 263, [560] = 262, [559] = 259, [558] = 258, [557] = 257, [556] = 256, 
  [555] = 253, [554] = 252, [553] = 251, [552] = 250, [551] = 284, [550] = 278, [549] = 277, [548] = 283, [547] = 282, [546] = 276, [545] = 275, 
  [544] = 281, [543] = 280, [542] = 274, [541] = 273, [540] = 279, [539] = 272, [538] = 278, [537] = 277, [536] = 271, [535] = 270, [534] = 276, 
  [533] = 275, [532] = 269, [531] = 268, [530] = 274, [529] = 273, [528] = 267, [527] = 272, [526] = 266, [525] = 261, [524] = 271, [523] = 270, 
  [522] = 260, [521] = 255, [520] = 269, [519] = 268, [518] = 254, [517] = 249, [516] = 267, [515] = 266, [514] = 265, [513] = 264, [512] = 263, 
  [511] = 262, [510] = 261, [509] = 260, [508] = 259, [507] = 258, [506] = 257, [505] = 256, [504] = 255, [503] = 254, [502] = 253, [501] = 252, 
  [500] = 251, [499] = 250, [498] = 249, [497] = 238, [496] = 239, [495] = 124, [494] = 133, [493] = 9, [492] = 10, [491] = 248, [490] = 248, [489] = 247, 
  [488] = 247, [487] = 246, [486] = 246, [485] = 245, [484] = 245, [483] = 244, [482] = 244, [481] = 243, [480] = 243, [479] = 242, [478] = 242, 
  [477] = 241, [476] = 241, [475] = 240, [474] = 240, [473] = 239, [472] = 238, [471] = 237, [470] = 237, [469] = 236, [468] = 236, [467] = 235, 
  [466] = 235, [465] = 234, [464] = 234...}

I'm not sure what should be done about this, but clearly there is some inconsistency in the geometry now.

kpedro88 · 2016-11-19T06:19:05Z

I think this must be caused by the change in HGCalDDDConstants::waferInLayer(). There are a number of wafers that now have in true which previously didn't. @bsunanda, are these changes intentional? If so, https://github.com/cms-data/L1Trigger-L1THGCal/blob/master/module_mapping.txt needs an update.

Correct reconstruction geometry for EE and FH with full coverage

d3d8edb

cmsbuild added this to the Next CMSSW_8_1_X milestone Nov 8, 2016

cmsbuild added comparison-pending dqm-pending geometry-pending orp-pending pending-signatures tests-pending labels Nov 8, 2016

cmsbuild added tests-started and removed tests-pending labels Nov 8, 2016

cmsbuild added tests-rejected and removed tests-started labels Nov 8, 2016

Correct the use of eras

b0da39f

cmsbuild added reconstruction-pending tests-pending and removed tests-rejected labels Nov 8, 2016

cmsbuild added tests-started and removed tests-pending labels Nov 8, 2016

cmsbuild removed the tests-started label Nov 9, 2016

cmsbuild added the geometry-approved label Nov 9, 2016

cmsbuild added dqm-approved and removed dqm-pending labels Nov 15, 2016

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Nov 15, 2016

slava77 reviewed Nov 15, 2016

View reviewed changes

cmsbuild mentioned this pull request Nov 16, 2016

bsunanda:Phase2-hgx67 Correct reconstruction geometry for EE and FH with full coverage #16608

Merged

cmsbuild added orp-approved and removed orp-pending labels Nov 17, 2016

cmsbuild merged commit 55add0c into cms-sw:CMSSW_8_1_X Nov 17, 2016

davidlange6 mentioned this pull request Nov 19, 2016

Revert "bsunanda:Phase2-hgx67 Correct reconstruction geometry for EE and FH with full coverage" #16694

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bsunanda:Phase2-hgx67 Correct reconstruction geometry for EE and FH with full coverage #16524

bsunanda:Phase2-hgx67 Correct reconstruction geometry for EE and FH with full coverage #16524

bsunanda commented Nov 8, 2016

cmsbuild commented Nov 8, 2016

bsunanda commented Nov 8, 2016

cmsbuild commented Nov 8, 2016 •

edited

cmsbuild commented Nov 8, 2016

cmsbuild commented Nov 8, 2016

cmsbuild commented Nov 8, 2016

bsunanda commented Nov 8, 2016

cmsbuild commented Nov 8, 2016

cmsbuild commented Nov 8, 2016 •

edited

cmsbuild commented Nov 9, 2016

cmsbuild commented Nov 9, 2016

bsunanda commented Nov 10, 2016

ianna commented Nov 11, 2016

smuzaffar commented Nov 11, 2016

bsunanda commented Nov 14, 2016

slava77 commented Nov 14, 2016

bsunanda commented Nov 15, 2016

bsunanda commented Nov 15, 2016

dmitrijus commented Nov 15, 2016

slava77 commented Nov 15, 2016

cmsbuild commented Nov 15, 2016

slava77 Nov 15, 2016

kpedro88 Nov 15, 2016

bsunanda Nov 16, 2016

bsunanda Nov 16, 2016

kpedro88 commented Nov 17, 2016

davidlange6 commented Nov 17, 2016

slava77 commented Nov 17, 2016

davidlange6 commented Nov 17, 2016

slava77 commented Nov 18, 2016

kpedro88 commented Nov 19, 2016

kpedro88 commented Nov 19, 2016

bsunanda:Phase2-hgx67 Correct reconstruction geometry for EE and FH with full coverage #16524

bsunanda:Phase2-hgx67 Correct reconstruction geometry for EE and FH with full coverage #16524

Conversation

bsunanda commented Nov 8, 2016

cmsbuild commented Nov 8, 2016

bsunanda commented Nov 8, 2016

cmsbuild commented Nov 8, 2016 • edited

cmsbuild commented Nov 8, 2016

cmsbuild commented Nov 8, 2016

cmsbuild commented Nov 8, 2016

bsunanda commented Nov 8, 2016

cmsbuild commented Nov 8, 2016

cmsbuild commented Nov 8, 2016 • edited

cmsbuild commented Nov 9, 2016

cmsbuild commented Nov 9, 2016

bsunanda commented Nov 10, 2016

ianna commented Nov 11, 2016

smuzaffar commented Nov 11, 2016

bsunanda commented Nov 14, 2016

slava77 commented Nov 14, 2016

bsunanda commented Nov 15, 2016

bsunanda commented Nov 15, 2016

dmitrijus commented Nov 15, 2016

slava77 commented Nov 15, 2016

cmsbuild commented Nov 15, 2016

slava77 Nov 15, 2016

Choose a reason for hiding this comment

kpedro88 Nov 15, 2016

Choose a reason for hiding this comment

bsunanda Nov 16, 2016

Choose a reason for hiding this comment

bsunanda Nov 16, 2016

Choose a reason for hiding this comment

kpedro88 commented Nov 17, 2016

davidlange6 commented Nov 17, 2016

slava77 commented Nov 17, 2016

davidlange6 commented Nov 17, 2016

slava77 commented Nov 18, 2016

kpedro88 commented Nov 19, 2016

kpedro88 commented Nov 19, 2016

cmsbuild commented Nov 8, 2016 •

edited

cmsbuild commented Nov 8, 2016 •

edited