Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[HGCAL] Corrections in noise vector in CLUE #32021

Merged
merged 7 commits into from
Nov 10, 2020
Merged

[HGCAL] Corrections in noise vector in CLUE #32021

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ad4acda
access noise same as the other vectors
apsallid Nov 2, 2020
49c1fc0
a comment to remember the extension
apsallid Nov 3, 2020
9555756
code checks
apsallid Nov 4, 2020
feb70d5
correcting HFNose out of range
apsallid Nov 4, 2020
7d64f80
droping type specifications
apsallid Nov 5, 2020
35d775c
use a cleaner syntax for the python config file
apsallid Nov 5, 2020
ed0b9b6
assign value of the object avoiding bindings
apsallid Nov 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 3 additions & 10 deletions RecoLocalCalo/HGCalRecProducers/plugins/HGCalCLUEAlgo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,15 +45,10 @@ class HGCalCLUEAlgoT : public HGCalClusteringAlgoBase {
maxNumberOfThickIndices_(ps.getParameter<unsigned>("maxNumberOfThickIndices")),
fcPerMip_(ps.getParameter<std::vector<double>>("fcPerMip")),
fcPerEle_(ps.getParameter<double>("fcPerEle")),
nonAgedNoises_(ps.getParameter<edm::ParameterSet>("noises").getParameter<std::vector<double>>("values")),
nonAgedNoises_(ps.getParameter<std::vector<double>>("noises")),
noiseMip_(ps.getParameter<edm::ParameterSet>("noiseMip").getParameter<double>("noise_MIP")),
use2x2_(ps.getParameter<bool>("use2x2")),
initialized_(false) {
// repeat same noises for CE-H as well
if (!isNose_) {
nonAgedNoises_.insert(std::end(nonAgedNoises_), std::begin(nonAgedNoises_), std::end(nonAgedNoises_));
}
}
initialized_(false) {}

~HGCalCLUEAlgoT() override {}

Expand Down Expand Up @@ -107,9 +102,7 @@ class HGCalCLUEAlgoT : public HGCalClusteringAlgoBase {
iDesc.add<unsigned>("maxNumberOfThickIndices", 6);
iDesc.add<std::vector<double>>("fcPerMip", {});
iDesc.add<double>("fcPerEle", 0.0);
edm::ParameterSetDescription descNestedNoises;
descNestedNoises.add<std::vector<double>>("values", {});
iDesc.add<edm::ParameterSetDescription>("noises", descNestedNoises);
iDesc.add<std::vector<double>>("noises", {});
edm::ParameterSetDescription descNestedNoiseMIP;
descNestedNoiseMIP.add<bool>("scaleByDose", false);
descNestedNoiseMIP.add<unsigned int>("scaleByDoseAlgo", 0);
Expand Down
10 changes: 7 additions & 3 deletions RecoLocalCalo/HGCalRecProducers/python/hgcalLayerClusters_cff.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,21 @@

from RecoLocalCalo.HGCalRecProducers.HGCalUncalibRecHit_cfi import HGCalUncalibRecHit

from SimCalorimetry.HGCalSimProducers.hgcalDigitizer_cfi import fC_per_ele, hgceeDigitizer, hgchebackDigitizer, hfnoseDigitizer
from SimCalorimetry.HGCalSimProducers.hgcalDigitizer_cfi import fC_per_ele, HGCAL_noises, hgceeDigitizer, hgchebackDigitizer, hfnoseDigitizer

hgcalLayerClusters = hgcalLayerClusters_.clone()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I suggest the possibly cleaner (I hope the syntax is still correct...):

Suggested change
hgcalLayerClusters = hgcalLayerClusters_.clone()
hgcalLayerClusters = hgcalLayerClusters_.clone(
timeOffset = hgceeDigitizer.tofDelay,
plugin = dict(
dEdXweights = dEdX.weights,
#With the introduction of 7 regional factors (6 for silicon plus 1 for scintillator),
#we extend fcPerMip (along with noises below) so that it is guaranteed that they have 6 entries.
fcPerMip = HGCalUncalibRecHit.HGCEEConfig.fCPerMIP + HGCalUncalibRecHit.HGCHEFConfig.fCPerMIP,
thicknessCorrection = HGCalRecHit.thicknessCorrection,
sciThicknessCorrection = HGCalRecHit.sciThicknessCorrection,
deltasi_index_regemfac = HGCalRecHit.deltasi_index_regemfac,
fcPerEle = fC_per_ele,
#Extending noises as fcPerMip, see comment above.
noises = HGCAL_noises.values + HGCAL_noises.values,
noiseMip = hgchebackDigitizer.digiCfg.noise
)
)


hgcalLayerClusters.timeOffset = hgceeDigitizer.tofDelay
hgcalLayerClusters.plugin.dEdXweights = cms.vdouble(dEdX.weights)
#With the introduction of 7 regional factors (6 for silicon plus 1 for scintillator),
#we extend fcPerMip (along with noises below) so that it is guaranteed that they have 6 entries.
hgcalLayerClusters.plugin.fcPerMip = cms.vdouble(HGCalUncalibRecHit.HGCEEConfig.fCPerMIP + HGCalUncalibRecHit.HGCHEFConfig.fCPerMIP)
hgcalLayerClusters.plugin.thicknessCorrection = cms.vdouble(HGCalRecHit.thicknessCorrection)
hgcalLayerClusters.plugin.sciThicknessCorrection = HGCalRecHit.sciThicknessCorrection
hgcalLayerClusters.plugin.deltasi_index_regemfac = HGCalRecHit.deltasi_index_regemfac
hgcalLayerClusters.plugin.fcPerEle = cms.double(fC_per_ele)
hgcalLayerClusters.plugin.noises = cms.PSet(refToPSet_ = cms.string('HGCAL_noises'))
#Extending noises as fcPerMip, see comment above.
hgcalLayerClusters.plugin.noises = cms.vdouble(HGCAL_noises.values + HGCAL_noises.values)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
hgcalLayerClusters.plugin.noises = cms.vdouble(HGCAL_noises.values + HGCAL_noises.values)
hgcalLayerClusters.plugin.noises = HGCAL_noises.values + HGCAL_noises.values

please drop type specifications for all parameters which already exist.
This is a safer way to protect from parameter name mistakes and will also help in possible parameter migrations.

it looks like this file did not make it to #30147 and the "drop type specs" migration task:

  • in the lines above the hgcalLayerClusters = hgcalLayerClusters_.clone() appears separately from the parameters modified later
  • in the lines below inside a clone nHitsTime = cms.uint32(3) from the old code still appears with type details

@jeongeun please check/clarify what happened.

I made a few comments to this file mainly for the updated lines, but it looks like our migration missed this file somehow

hgcalLayerClusters.plugin.noiseMip = hgchebackDigitizer.digiCfg.noise

hgcalLayerClustersHFNose = hgcalLayerClusters_.clone(
Expand All @@ -26,10 +29,11 @@
nHitsTime = cms.uint32(3),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
nHitsTime = cms.uint32(3),
nHitsTime = 3,

or can be dropped because it's the same in the original hgcalLayerClusters_

plugin = dict(
dEdXweights = dEdX.weightsNose,
maxNumberOfThickIndices = cms.uint32(3),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
maxNumberOfThickIndices = cms.uint32(3),
maxNumberOfThickIndices = 3,

fcPerMip = HGCalUncalibRecHit.HGCHFNoseConfig.fCPerMIP,
thicknessCorrection = HGCalRecHit.thicknessNoseCorrection,
fcPerEle = fC_per_ele,
noises = dict(refToPSet_ = cms.string('HGCAL_noises')),
noises = cms.vdouble(HGCAL_noises.values),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
noises = cms.vdouble(HGCAL_noises.values),
noises = HGCAL_noises.values.value(),

please drop type specifications for all parameters which already exist.
This is a safer way to protect from parameter name mistakes and will also help in possible parameter migrations.

noiseMip = hgchebackDigitizer.digiCfg.noise
)
)