flag to allow the possibility to activate the ZS offline applied to all crystals

[amassiro]

Allow ECAL pfrechit thresholds eta dependent in all regions, disregarding if they have been read in Zero Suppression (ZS) of Full Readout (FR).

In the previous version we could activate these thresholds only in ZS regions, or de-activate the application of the thresholds everywhere.

With this PR we can activate the thresholds everywhere, by means of the boolean: "applySelectionsToAllCrystals"

[cmsbuild]

A new Pull Request was created by @amassiro (Andrea Massironi) for CMSSW_10_0_X.

It involves the following packages:

RecoParticleFlow/PFClusterProducer

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@mmarionncern, @felicepantaleo, @rovere, @lgray, @seemasharmafnal, @cbernet, @bachtis this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

• Backported from Allow ECAL pfrechit thresholds eta dependent in all regions, disregarding if ZS of FR (10-1-X) #22265

[amassiro]

@argiro @crovelli @emanueledimarco

[slava77]

the same functionality can already be achieved and is already used by using srFlags = cms.InputTag("")
What is new functionally in this PR?

Please note that the feature has to be integrated in the master branch first.
Also, please justify the need to update 10_0_X

[amassiro]

Unfortunately, that is not the case.

If we set:
cms.InputTag("")
the thresholds are never applied.
This is due to the fact that just setting "" the test:
return fullReadOut or pass(hit);
https://github.com/amassiro/cmssw/blob/ce2ffce36b59010e8fbc2b7c52a0f8098440dc63/RecoParticleFlow/PFClusterProducer/interface/PFRecHitQTests.h#L29

will be always true, since "fullReadout" is true, being set by:
https://github.com/amassiro/cmssw/blob/ce2ffce36b59010e8fbc2b7c52a0f8098440dc63/RecoParticleFlow/PFClusterProducer/interface/PFEcalBarrelRecHitCreator.h#L67
bool hi = (useSrF ? isHighInterest(detid) : true);

(and similarly in EE rechit producer).

This has been observed "experimentally". I generated with high thresholds, and they were not applied.
With this fix, and setting the flag to "true" the thresholds are applied.

[slava77]

Unfortunately, that is not the case.

Isn't the problem then in this line

bool hi = (useSrF ? isHighInterest(detid) : true);

that useSrF false should lead to "false" so that normal cuts are applied?

"useSrF" is a property of a full job, not something changing hit by hit.
In case useSrF is false, falling to "hi = true" is meaningless because it disables any selections completely.
So, this is apparently a bug.
We should fix this instead of introducing a workaround.

[amassiro]

We want to intorduce a new feature.

The current version has:

1. apply threshold only in ZS thresholds -> if I define "useSrF" with some input tag
2. do not apply thresholds -> if I define "useSrF" as ""
3. apply threshold to all rechits -> by setting the newly introduced flag [this possibility was not available in the past]

I tested before what you are proposing
bool hi = (useSrF ? isHighInterest(detid) : false);
and it definitely has the same effect of the current implementation, but it will remove the option (2).
I tried to maintain back-compatibility of the code.
I let @emanueledimarco comment on the details of why the code was written in such a way.

[slava77]

On 2/16/18 9:05 AM, Andrea Massironi wrote: We want to intorduce a new feature. The current version has: 1. apply threshold only in ZS thresholds -> if I define "useSrF" with some input tag 2. do not apply thresholds -> if I define "useSrF" as ""

this can be achieved by setting empty thresholds or not using a QTest at all. I don't see a clear need to have a QTest that's known at configuration level to do nothing.

…

3. apply threshold to all rechits -> by setting the newly introduced flag [this possibility was not available in the past] I tested before what you are proposing bool hi = (useSrF ? isHighInterest(detid) : false); and it definitely has the same effect of the current implementation, but it will remove the option (2). I tried to maintain back-compatibility of the code. I let @emanueledimarco <https://github.com/emanueledimarco> comment on the details of why the code was written in such a way. —

[slava77]

The other aspect of this PR is that there is a bug in 10_0_X in PF selections.
Because the current thresholds of 0.08 EB and 0.3 EE do nothing due to this srFlags = "" not doing what we (at least me) mistakenly thought it should.
Did the ECAL RelVal validation sign off on this as OK for the latest 10_0_X?

[amassiro]

The gathering and seeding thresholds are >= than the ones defined here, done on purpose since the study of the different thresholds and effects on high level objects is still ongoing. But without this fix, or the one you propose, it cannot be performed.

The only effect here is the threshold applied to the pf-rechits that are not clustered.
Though I let object experts comments on their specific use of unclustered components (maybe in the isolation cone definition? Unclustered met?)

[slava77]

So, my preference now is to change the true to false in the absence of SR info.
Also, we need a master PR first.

[slava77]

@cmsbuild please test

[cmsbuild]

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/26136/console Started: 2018/02/16 19:11

[slava77]

@amassiro

I'll run locally with "false" to see what the impact is.
If it's small, perhaps we fix it in the master and leave 10_0_X with your solution here which does not change the selection logic in PFEcalBarrelRecHitCreator

[cmsbuild]

-1

Tested at: ce2ffce

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

I found follow errors while testing this PR

Failed tests: RelVals AddOn

• RelVals:

When I ran the RelVals I found an error in the following worklfows:
10824.0 step2

runTheMatrix-results/10824.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2018_GenSimFull+DigiFull_2018+RecoFull_2018+ALCAFull_2018+HARVESTFull_2018/step2_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2018_GenSimFull+DigiFull_2018+RecoFull_2018+ALCAFull_2018+HARVESTFull_2018.log

• AddOn:

I found errors in the following addon tests:

cmsRun /cvmfs/cms-ib.cern.ch/nweek-02511/slc6_amd64_gcc630/cms/cmssw-patch/CMSSW_10_0_X_2018-02-16-1100/src/HLTrigger/Configuration/test/OnLine_HLT_PRef.py realData=True globalTag=@ inputFiles=@ : FAILED - time: date Fri Feb 16 20:16:30 2018-date Fri Feb 16 20:14:16 2018 s - exit: 18688
cmsDriver.py RelVal -s HLT:PRef,RAW2DIGI,L1Reco,RECO --data --scenario=pp -n 10 --conditions auto:run2_data_PRef --relval 9000,50 --datatier "RAW-HLT-RECO" --eventcontent FEVTDEBUGHLT --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --era Run2_2018 --processName=HLTRECO --filein file:RelVal_Raw_PRef_DATA.root --fileout file:RelVal_Raw_PRef_DATA_HLT_RECO.root : FAILED - time: date Fri Feb 16 20:16:30 2018-date Fri Feb 16 20:14:16 2018 s - exit: 18688
cmsRun /cvmfs/cms-ib.cern.ch/nweek-02511/slc6_amd64_gcc630/cms/cmssw-patch/CMSSW_10_0_X_2018-02-16-1100/src/HLTrigger/Configuration/test/OnLine_HLT_GRun.py realData=False globalTag=@ inputFiles=@ : FAILED - time: date Fri Feb 16 20:26:43 2018-date Fri Feb 16 20:14:21 2018 s - exit: 18688
cmsDriver.py RelVal -s HLT:GRun,RAW2DIGI,L1Reco,RECO --mc --scenario=pp -n 10 --conditions auto:run2_mc_GRun --relval 9000,50 --datatier "RAW-HLT-RECO" --eventcontent FEVTDEBUGHLT --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --era Run2_2018 --processName=HLTRECO --filein file:RelVal_Raw_GRun_MC.root --fileout file:RelVal_Raw_GRun_MC_HLT_RECO.root : FAILED - time: date Fri Feb 16 20:26:43 2018-date Fri Feb 16 20:14:21 2018 s - exit: 18688
cmsRun /cvmfs/cms-ib.cern.ch/nweek-02511/slc6_amd64_gcc630/cms/cmssw-patch/CMSSW_10_0_X_2018-02-16-1100/src/HLTrigger/Configuration/test/OnLine_HLT_PRef.py realData=False globalTag=@ inputFiles=@ : FAILED - time: date Fri Feb 16 20:26:25 2018-date Fri Feb 16 20:14:28 2018 s - exit: 18688
cmsDriver.py RelVal -s HLT:PRef,RAW2DIGI,L1Reco,RECO --mc --scenario=pp -n 10 --conditions auto:run2_mc_PRef --relval 9000,50 --datatier "RAW-HLT-RECO" --eventcontent FEVTDEBUGHLT --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --era Run2_2018 --processName=HLTRECO --filein file:RelVal_Raw_PRef_MC.root --fileout file:RelVal_Raw_PRef_MC_HLT_RECO.root : FAILED - time: date Fri Feb 16 20:26:25 2018-date Fri Feb 16 20:14:28 2018 s - exit: 18688
cmsRun /cvmfs/cms-ib.cern.ch/nweek-02511/slc6_amd64_gcc630/cms/cmssw-patch/CMSSW_10_0_X_2018-02-16-1100/src/HLTrigger/Configuration/test/OnLine_HLT_GRun.py realData=True globalTag=@ inputFiles=@ : FAILED - time: date Fri Feb 16 20:21:01 2018-date Fri Feb 16 20:14:56 2018 s - exit: 18688
cmsDriver.py RelVal -s HLT:GRun,RAW2DIGI,L1Reco,RECO --data --scenario=pp -n 10 --conditions auto:run2_data_GRun --relval 9000,50 --datatier "RAW-HLT-RECO" --eventcontent FEVTDEBUGHLT --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --era Run2_2018 --processName=HLTRECO --filein file:RelVal_Raw_GRun_DATA.root --fileout file:RelVal_Raw_GRun_DATA_HLT_RECO.root : FAILED - time: date Fri Feb 16 20:21:01 2018-date Fri Feb 16 20:14:56 2018 s - exit: 18688

[cmsbuild]

Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped)

[slava77]

@amassiro

the failure in the test is due to hltParticleFlowRecHitECALUnseeded
This new parameter will need a customization.

After some thought, I'm convinced now that your solution is more appropriate in a bigger picture:

• it is more appropriate to make the choice of ignoring the SR flags in a specific PFRecHitQTests
• the PFEcalBarrelRecHitCreator should not decide for every possible QTest what to do with the SR flags

Eventually, the full solution will be to

• restore reading of SRFlags in PFEcalBarrelRecHitCreator
• set the PFRecHitQTestECALMultiThreshold's applySelectionsToAllCrystals to true

This will actually restore the selections to what we had in 2016 as originally intended for 10_0_X; and it will also allow to still have extra flexibility if needed to apply other quality tests using SRFlags.

[slava77]

To follow up on #22254 (comment)
and #22254 (comment)

I see that the bug is latent in the current default configuration of PF: the extra PFRecHits are never used or are discarded by downstream code because the thresholds downstream are at least as high as the intended selections that had to be applied in particleFlowRecHitECAL.

So, there is nothing to fix in 10_0_X at this point.

@amassiro once you have a fix for the HLT parameters, please also make a PR to the master branch.

[Martin-Grunewald]

+1

[cmsbuild]

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

Comparison Summary:

• No significant changes to the logs found
• Reco comparison results: 0 differences found in the comparisons
• DQMHistoTests: Total files compared: 27
• DQMHistoTests: Total histograms compared: 2469334
• DQMHistoTests: Total failures: 1
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 2469164
• DQMHistoTests: Total skipped: 169
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 1.30000000015 KiB( 22 files compared)
• Checked 111 log files, 9 edm output root files, 27 DQM output files

[slava77]

backport of #22265

[slava77]

+1

for #22254 afd050a

• fixes a latent bug in PF{EB,EE}RecHitCreator that didn't allow changing thresholds in PFRecHitQTestECALMultiThreshold; also introduces new alternative test scenarios for ECAL thresholds to be used first within TSG
• the changes are the same as in Allow ECAL pfrechit thresholds eta dependent in all regions, disregarding if ZS of FR (10-1-X) #22265 (ignoring some empty lines changes)
• jenkins tests pass and comparisons with the baseline show no differences

[cmsbuild]

This pull request is fully signed and it will be integrated in one of the next CMSSW_10_0_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_10_1_X is complete. 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)

[Martin-Grunewald]

@fabiocos could you please merge?
Thanks!

[fabiocos]

+1

[Martin-Grunewald]

Hi @amassiro

Just to check: the HLT customisation is NOT applied to hltParticleFlowRecHitECALForMuonsMFNoVtx
Is this correct? You only apply it to:
['hltParticleFlowRecHitECALUnseeded', 'hltParticleFlowRecHitECALL1Seeded', 'hltParticleFlowRecHitECALForMuonsMF', 'hltParticleFlowRecHitECALForTkMuonsMF']

[amassiro]

"hltParticleFlowRecHitECALForMuonsMFNoVtx" is using a different QTest:

      name = cms.string( "PFRecHitQTestThreshold" )

We have not changed it, since it was as this also in 2017.

We have changed all the producers whose QTest in 2017 was "PFRecHitQTestECALMultiThreshold"

[Martin-Grunewald]

What does this comment mean in the HLT customisation? Something forgotten or to be done later in another PR?

# particleFlowRechitECAL new default value "false" flag to be added

[amassiro]

Something forgotten, sorry about it.

[Martin-Grunewald]

Depending on what it is and for both 10_0 and 10_1 we can do it directly in ConfDB rather than via PRs and customisation functions. It depends what the change is!

[amassiro]

In this PR we have both introduced the possibility to apply selections at pf-rechit level eta dependent, and we have defined a set of reasonable thresholds that will be studied by POGs for performance of different objects.

The change in the code requires an additional input tag to be handled by the producer (a simple bool) that was not there in the HLT part and that would make the relval matrix crash without the customization. That is why we added the customization function.
If this can be fixed once and for all via ConfDB as well, this is very good.

[Martin-Grunewald]

Additional parameters are trivially to handle with a fillDescriptions method (for all parameters).
Then you could just add it in the code, with a defaults value, and be done with it.