bsunanda:Run2-hcx116 back port PR 17948 for treatment of ND filter

[bsunanda]

This is needed for correcting GEN-SIM step as well

[bsunanda]

@cmsbuild Please test

[cmsbuild]

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/18469/console Started: 2017/03/16 00:11

[cmsbuild]

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

It involves the following packages:

Geometry/HcalCommonData
SimG4CMS/Calo

@civanch, @Dr15Jones, @ianna, @mdhildreth, @cmsbuild, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @makortel this is something you requested to watch as well.
@davidlange6, @smuzaffar you are the release manager for this.

cms-bot commands are listed here #13028

[cmsbuild]

+1
Tested at: 2b20bbc
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-17949/18469/summary.html

[cmsbuild]

Comparison job queued.

[cmsbuild]

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

[ianna]

+1

Note: in some cases depth == 0 becomes depth == 1

[civanch]

@bsunanda , all 2017 WFs demonstrate substantial differences, is this PR a bug fix? I would extend description writing that explicitly, ideally describing what means depth 0, 1, 2

There are 3 identical peaces of the code : lines 639-644, 959-961, 1022-1027:
if (numberingFromDDD) {
HcalNumberingFromDDD::HcalID tmp = numberingFromDDD->unitID(det,etaR,phi,depth,1);
modifyDepth(tmp);
if (numberingScheme) unitID = numberingScheme->getUnitID(tmp);
}

I would change this to

if(numberingFromDDD && numberingScheme) {
HcalNumberingFromDDD::HcalID tmp = numberingFromDDD->unitID(det,etaR,phi,depth,1);
modifyDepth(tmp);
unitID = numberingScheme->getUnitID(tmp);
}
or include this block into modifyDepth(det,etaR,phi,depth,1) to avoid code duplication.

There another block lines 1100-1145, where 'numberingScheme' flag is not check. Is this done for purpose?

[cmsbuild]

Pull request #17949 was updated. @civanch, @Dr15Jones, @ianna, @mdhildreth, @cmsbuild, @davidlange6 can you please check and sign again.

[bsunanda]

@cmsbuild Please test

[cmsbuild]

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/18490/console Started: 2017/03/16 15:13

[cmsbuild]

+1
Tested at: 3f58239
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-17949/18490/summary.html

[cmsbuild]

Comparison job queued.

[ianna]

+1

[cmsbuild]

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

[abdoulline]

Quick check with 2017 ("Plan1") single-pion gun :

(1) without this PR: an issue of HcalRecHits scale (expected SimHits change isn't compensated downstream) when compared to Phase0 (2016)
https://cms-cpt-software.web.cern.ch/cms-cpt-software/General/Validation/SVSuite/HCAL/calo_scan_single_pi/900pre6_Plan1_vs_900pre6_SinglePi/

(2) with this PR: HcalRecHits become OK
https://cms-cpt-software.web.cern.ch/cms-cpt-software/General/Validation/SVSuite/HCAL/calo_scan_single_pi/900pre6_Plan1fix_vs_900pre6_SinglePi/

[civanch]

@bsunanda , @abdoulline , I am ready to sign but do not understand the a minor thing: line 1106 of the SD code: here you modify depth without checking 'numberingScheme' flag. In all other places both 'numberingScheme' and 'numberingFromDDD' are checked before "modify". Is this intended?

[bsunanda]

@civanch We have the option of giving layer weights based on some measurements from source scan results. This is done through files which provide eta, phi, depth indices and they are saved by packing using testNumberingScheme within HCalSD code and this is used while storing hits. So for this we know which packing is required. That is why there is no test of packing scheme is made there.

[bsunanda]

@civanch Please approve this. We need this for 2017 SIM step

[civanch]

+1

[cmsbuild]

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

[davidlange6]

+1