[HGCAL] Corrections in noise vector in CLUE

[apsallid]

PR description:

With the introduction of 7 regional factors (6 for silicon plus 1 for scintillator), the fcPerMip and
noises vectors should be guaranteed that they have 6 entries. We alter the way the noises vector is extended, since it uses the isNose_ member variable before the variable is set, while the insert method should not be called using iterators that refer to the very same vector we are trying to extend.

PR validation:

No difference is observed to the printout information.

if this PR is a backport please specify the original PR and why you need to backport that PR:

Not a backport.

[cmsbuild]

The code-checks are being triggered in jenkins.

[cmsbuild]

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32021/19561

• This PR adds an extra 16KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

• code-format:
  https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32021/19561/code-format.patch
  e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32021/19561/code-format.patch | patch -p1
  You can also run scram build code-format to apply code format directly

[cmsbuild]

The code-checks are being triggered in jenkins.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32021/19563

• This PR adds an extra 16KB to repository

• There are other open Pull requests which might conflict with changes you have proposed:

  • File RecoLocalCalo/HGCalRecProducers/plugins/HGCalCLUEAlgo.h modified in PR(s): HGCAL: restore distance cut in CLUE position calculation #32022

[cmsbuild]

A new Pull Request was created by @apsallid for master.

It involves the following packages:

RecoLocalCalo/HGCalRecProducers

@perrotta, @jpata, @cmsbuild, @kpedro88, @slava77 can you please review it and eventually sign? Thanks.
@edjtscott, @vandreev11, @deguio, @sethzenz, @felicepantaleo, @riga, @rovere, @lgray, @cseez, @pfs, @lecriste, @hatakeyamak, @clelange this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

[rovere]

This should fix #31940 for HGCAL.

On a side note: are we sure that also the HFNose case is correctly handled? That all the input collections have the correct size and that the C++ code is able to handle all cases?
I'm not quite sure this is the case. Please, test an HFNose workflow.
@mariadalfonso

[cmsbuild]

The code-checks are being triggered in jenkins.

[apsallid]

@rovere @mariadalfonso With the operator [] no problem is observed, however using .at(location) which also checks for the range an exception is thrown. So, I changed the relevant parameter which for HGCAL is 6 to 3 for HFNose to avoid the crash but then switch back to the faster operator[].

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32021/19568

• This PR adds an extra 16KB to repository

• There are other open Pull requests which might conflict with changes you have proposed:

  • File RecoLocalCalo/HGCalRecProducers/plugins/HGCalCLUEAlgo.h modified in PR(s): HGCAL: restore distance cut in CLUE position calculation #32022

[cmsbuild]

+1
Tested at: ed0b9b6
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-75a1a8/10545/summary.html
CMSSW: CMSSW_11_2_X_2020-11-06-1100
SCRAM_ARCH: slc7_amd64_gcc820

[cmsbuild]

Comparison job queued.

[cmsbuild]

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

Comparison Summary:

• No significant changes to the logs found
• Reco comparison results: 4 differences found in the comparisons
• DQMHistoTests: Total files compared: 35
• DQMHistoTests: Total histograms compared: 2544144
• DQMHistoTests: Total failures: 7
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 2544115
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
• Checked 149 log files, 22 edm output root files, 35 DQM output files

[perrotta]

From RecoLocalCalo/HGCalRecProducers/plugins/HGCalCLUEAlgo.cc:

  // To support the TDR geometry and also the post-TDR one (v9 onwards), we
  // need to change the logic of the vectors containing signal to noise and
  // thresholds. The first 3 indices will keep on addressing the different
  // thicknesses of the Silicon detectors in CE_E , the next 3 indices will address
  // the thicknesses of the Silicon detectors in CE_H, while the last one, number 6 (the
  // seventh) will address the Scintillators. This change will support both
  // geometries at the same time.

It implies that the vectors fcPerMip, noises, thicknessCorrection must have dimension 6 (3+3).

thicknessCorrection was already imported with the correct dimension from RecoLocalCalo/HGCalRecProducers/python/HGCalRecHit_cfi.py

fcPerMip and noises were previously imported from the corresponding vectors of dimension 3 in SimCalorimetry/HGCalSimProducers/python/hgcalDigitizer_cfi.py, and this is what generated the issue reported in #31940

This PR imports twice in the hgcalLayerClusters configuration the same vectors fcPerMip and noises, so that the correct dimensionality is restored. In spite of it being a bit complicated procedure, this should work as soon as the thicknesses of the three Silicon detectors in CE_E is the same as the thicknesses of the corresponding three Silicon detectors in CE_H: could you please confirm it? It is not foreseen that those thicknesses can be different at some point (in which case this procedure of duplicating identically fcPerMip and noises can easily lead to mistakes in future)?

For my own understanding: why the thicknessCorrection for the CE_H Silicon detectors is different (i.e. always "1") from the one of the CE_E Silicon detectors (see

cmssw/RecoLocalCalo/HGCalRecProducers/python/HGCalRecHit_cfi.py

Line 107 in ed0b9b6

thicknessCorrection = cms.vdouble(1.132,1.092,1.084, 1.0, 1.0, 1.0), # 100, 200, 300 um

)?

[apsallid]

Hi @perrotta ,

The physical thicknesses at CE_E is the same as the thicknesses in CE_H. Initially, HGCal used 3 thickness correction factors to equalize the signal scale in the detector, coming from the silicon in CE_E. However, later it was proven that 3 factors weren't adequate to equalize the signal in all detector. Silicon in the CE_H part needed different factors. The term we use now is regional em factors since from 3 we ended up using 7.

The "1" in the thicknessCorrection is because we didn't want to break the previous geometries work. The extra CE_H and scintillator factors were calculated in the HGCAL V10 geometry scenario, not before, which if you see in CE_E is above 1 and substantially different from what we use by now.

On your question:
It is not foreseen that those thicknesses can be different at some point (in which case this procedure of duplicating identically fcPerMip and noises can easily lead to mistakes in future)?

I know that @rovere is strongly considering the cleaning and reorganization of the code.
The idea was to push now this solution, since we know that the code as is now needs the fix.

[perrotta]

+1

• Vectors of size 6 were previously inserted as vector of size 3 in the hgcalLayerClusters configuration. This PR correctly imports them as vectors of size 6, and it solves as such the issue.
• The method of importing one 3-dimensional vector from hgcalDigitizer and inserting it twice to get the needed 6-dimensional one is not super clean, but it works and it is functional to the current setup. Moreover, comment lines in the code clearly indicate what was done, so that it can be easily taken into account in case of further updates.
  • However, it is understood that an overall reorganization of those configurations is in order and already planned by HGCal
• Jenkins tests pass

[kpedro88]

+upgrade

[cmsbuild]

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)

[qliphy]

+1