Adopt HFSignalAsymmetry flag in HF PFRecHit cleaning #33520

hatakeyamak · 2021-04-25T03:06:18Z

PR description:

HFSignalAsymmetry flag was introduced after the HF phase1 upgrade for noise flagging by HCAL DPG, but it was never adopted in PF cleaning (*). This PR adds this flag for HF PFRecHit cleaning.

(*) https://indico.cern.ch/event/944298/timetable/#10-pf-noise-cleaning-for-hcal

Also following discussions on #27767 and a request from @abdoulline we put the HBHENegativeNoise flag ineffective starting from Run 3 (this flag won't be created from Run 3, so it was not really necessary, but this change was made to avoid potential confusions).

PR validation:

We don't expect a large change by addition of HFSignalAsymmetry to HF PFRecHit cleaning. As seen in: https://indico.cern.ch/event/1027487/#36-hf-noise-flagging-for-pf, we do not see any concerning changes. If we may say, slight reduction in MET in MET PD which is expected to include some high MET events due to noise is probably a positive effect.

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

This is not a backport.

@bendavid @laurenhay @marksan87 @abdoulline @toropin

cmsbuild · 2021-04-25T03:14:40Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33520/22277

This PR adds an extra 24KB to repository

cmsbuild · 2021-04-25T03:15:06Z

A new Pull Request was created by @hatakeyamak (Kenichi Hatakeyama) for master.

It involves the following packages:

Configuration/DataProcessing
RecoParticleFlow/PFClusterProducer

@perrotta, @silviodonato, @cmsbuild, @franzoni, @slava77, @jpata, @qliphy, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks.
@mmarionncern, @felicepantaleo, @rovere, @lgray, @Martin-Grunewald, @clelange, @lecriste, @cbernet, @fabiocos, @seemasharmafnal 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

perrotta · 2021-04-26T07:22:16Z

@cmsbuild please test

silviodonato · 2021-04-26T09:05:52Z

cc @cms-sw/hgcal-dpg-l2

cmsbuild · 2021-04-26T10:04:02Z

-1

Failed Tests: UnitTests
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b6c772/14568/summary.html
COMMIT: bdee9b1
CMSSW: CMSSW_12_0_X_2021-04-25-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/33520/14568/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found errors in the following unit tests:

---> test TestConfigDP had ERRORS

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 1017 differences found in the comparisons
DQMHistoTests: Total files compared: 38
DQMHistoTests: Total histograms compared: 2877605
DQMHistoTests: Total failures: 1155
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2876427
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.016 KiB( 37 files compared)
DQMHistoSizes: changed ( 136.874 ): -0.016 KiB JetMET/SUSYDQM
Checked 160 log files, 37 edm output root files, 38 DQM output files
TriggerResults: no differences found

slava77 · 2021-04-27T16:30:29Z

cc @cms-sw/hgcal-dpg-l2

looks like a wrong DPG was tagged.

@cms-sw/hcal-dpg-l2

abdoulline · 2021-04-28T05:24:08Z

Kenichi has mentioned in the PR header/preamble a discussion with me and Alexander Toropin in #27767
I believe we've agreed on the changes kindly implemented by Kenichi in this PR.

Concening a particular aspect of the HBHENegativeNoise flag inactivation", I've suggested its removal
#27767 (comment)

Now, a propos de Unit Test failure on HBHENegativeNoise flag SeverityLevel modification -
I have the impression the test uses directly RecoTLR for customization, where now (in this PR) HBHENegativeNoise flag is supposed to be reset to SeverityLevel=8, exactly as it's defined in the source config: https://cmssdt.cern.ch/lxr/source/cfipython/RecoLocalCalo/HcalRecAlgos/hcalRecAlgos_cfi.py#0049
I'm not sure it would work...
So, I'd suggest to either
(i) remove the flag .RemoveFlag() or
(ii) change it to SeverityLevel=5 (for monitoring purposes ?).

slava77 · 2021-04-28T15:59:59Z

So, I'd suggest to either
(i) remove the flag .RemoveFlag() or
(ii) change it to SeverityLevel=5 (for monitoring purposes ?).

IIUC, the flag is already set to severity 12 in

cmssw/Configuration/StandardSequences/python/Reconstruction_Data_cff.py

Line 23 in bdee9b1

HcalRemoveAddSevLevel.AddFlag(hcalRecAlgos,"HBHENegativeNoise",12)

and it looks like the .AddFlag is broken in this case

cmssw/RecoLocalCalo/HcalRecAlgos/python/RemoveAddSevLevel.py

Lines 89 to 92 in bdee9b1

    
           else: 
        
               Flags.remove(flag) 
        
               # Removing flag leaves nothing else:  need to remove this level completely 
        
               if len(Flags)==0 and ChanStat==['']:

So, I guess either this part needs a fix or there should be a RemoveFlag first.
@abdoulline does my analysis look correct?

Perhaps a cleaner solution still is to use era/modifier in Configuration/StandardSequences/python/Reconstruction_Data_cff.py

hatakeyamak · 2021-04-28T16:07:28Z

Sorry. I am a little behind. I think (i) i.e. .RemoveFlag() should work (and local test looked good), but i wasn't convinced yet that it is the cleanest thing to do. So, I will think more, and a suggestion is certainly welcome.

abdoulline · 2021-04-28T17:47:53Z

@slava77
Not sure...

In a very much similar situation "HBHEFlatNoise" flag is also assigned 12 in Reconstruction_Data_cff.py

cmssw/Configuration/StandardSequences/python/Reconstruction_Data_cff.py

Line 22 in bdee9b1

HcalRemoveAddSevLevel.AddFlag(hcalRecAlgos,"HBHEFlatNoise",12)

And then changed back to 8 in RecoTLR.py and propagated customisePostEra_Run2_2017 -> customisePostEra_Run2_2018 -> customisePostEra_Run3 :

cmssw/Configuration/DataProcessing/python/RecoTLR.py

Line 49 in bdee9b1

HcalRemoveAddSevLevel.AddFlag(process.hcalRecAlgos,"HBHEFlatNoise",8)

And there were/is no issue (?)

I think Reconstruction_Data_cff.py may not be the best location for era-related modifications, as all those are present in RecoTLR.py (?)
I think the simplest is an option (i).

slava77 · 2021-04-28T18:15:42Z

And there were/is no issue (?)

It's likely that in

cmssw/RecoLocalCalo/HcalRecAlgos/python/RemoveAddSevLevel.py

Lines 89 to 92 in bdee9b1

    
           else: 
        
               Flags.remove(flag) 
        
               # Removing flag leaves nothing else:  need to remove this level completely 
        
               if len(Flags)==0 and ChanStat==['']:

the if len(Flags)==0 : is already false and ChanStat is never checked.

The error message in the test is pretty clear

  File ".../python/Configuration/DataProcessing/RecoTLR.py", line 94, in customise2021
    HcalRemoveAddSevLevel.AddFlag(process.hcalRecAlgos,"HBHENegativeNoise",8)
  File ".../python/RecoLocalCalo/HcalRecAlgos/RemoveAddSevLevel.py", line 92, in AddFlag
    if len(Flags)==0 and ChanStat==['']:
NameError: global name 'ChanStat' is not defined

It would be nicer to fix RemoveAddSevLevel.py

abdoulline · 2021-04-28T18:36:39Z

@slava77
RemoveAddSevLevel.py has been implemented ~10 years ago by a colleague who has left CMS since.
If you have a fix in mind, would be very kind of you to share it with Kenichi (who has asked about suggestions), so that he could add it to this PR (?)

slava77 · 2021-04-28T18:55:08Z

@slava77
RemoveAddSevLevel.py has been implemented ~10 years ago by a colleague who has left CMS since.
If you have a fix in mind, would be very kind of you to share it with Kenichi (who has asked about suggestions), so that he could add it to this PR (?)

I can propose to drop and ChanStat==[''] (which is perhaps wrong)
or to set it as it's done in the RemoveFlag ChanStat=sevLevelComputer.SeverityLevels[i].ChannelStatus.value()
but it's better to be done with some guidance by someone from HCAL.

I'm fine to have RemoveFlag -> AddFlag sequence of calls added in this PR, if we can't agree on modifying RemoveAddSevLevel.py

cmsbuild · 2021-04-29T15:18:41Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33520/22379

This PR adds an extra 16KB to repository

cmsbuild · 2021-04-29T15:19:02Z

Pull request #33520 was updated. @perrotta, @silviodonato, @cmsbuild, @franzoni, @slava77, @jpata, @qliphy, @fabiocos, @davidlange6 can you please check and sign again.

slava77 · 2021-04-29T15:36:00Z

@cmsbuild please test

cmsbuild · 2021-04-29T19:14:36Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b6c772/14710/summary.html
COMMIT: 10e7377
CMSSW: CMSSW_12_0_X_2021-04-29-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/33520/14710/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 1018 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2662700
DQMHistoTests: Total failures: 1162
DQMHistoTests: Total nulls: 2
DQMHistoTests: Total successes: 2661514
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.02 KiB( 36 files compared)
DQMHistoSizes: changed ( 136.874 ): -0.016 KiB JetMET/SUSYDQM
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 155 log files, 37 edm output root files, 37 DQM output files
TriggerResults: no differences found

abdoulline · 2021-04-29T20:17:13Z

Only 2017/2018 data exhibit some small PFlow -> downstream changes.
Seems to be what's expected due to HFSignalAsymmetry flag addition...

slava77 · 2021-04-30T01:20:34Z

+reconstruction

for #33520 10e7377

code changes are in line with the PR description and the follow up review
- a part of the changes is related to HF and can be visible in the tests
- the other part of this PR is going to be visible only with Run-3 data (future)
jenkins tests pass and comparisons with the baseline show small differences starting from particleFlowClusterHF e.g. wf 136.874 (the expected behavior is also confirmed in Adopt HFSignalAsymmetry flag in HF PFRecHit cleaning #33520 (comment))

silviodonato · 2021-04-30T08:14:11Z

+1

cmsbuild · 2021-04-30T08:14:33Z

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 be automatically merged.

hatakeyamak added 3 commits April 9, 2021 06:48

Use HFSignalAsymmetry for HF cleaning in PF.

0503979

use HFSignalAsymmetry enum

b89c1b1

Add customize2021. Put HBHE flags explicitly out of the game.

dbbdb3b

cmsbuild added this to the CMSSW_12_0_X milestone Apr 25, 2021

cmsbuild added code-checks-pending operations-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Apr 25, 2021

english consistency

bdee9b1

cmsbuild added code-checks-approved and removed code-checks-pending labels Apr 25, 2021

cmsbuild added tests-started and removed tests-pending labels Apr 26, 2021

cmsbuild added tests-rejected and removed tests-started labels Apr 26, 2021

cmsbuild added the tests-pending label Apr 29, 2021

Introduce _hcalCustomsRun3 part 2.

10e7377

cmsbuild added code-checks-pending and removed code-checks-approved labels Apr 29, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Apr 29, 2021

cmsbuild added tests-started and removed tests-pending labels Apr 29, 2021

cmsbuild added tests-approved and removed tests-started labels Apr 29, 2021

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Apr 30, 2021

cmsbuild added fully-signed operations-approved orp-approved and removed operations-pending orp-pending pending-signatures labels Apr 30, 2021

cmsbuild merged commit d5801d3 into cms-sw:master Apr 30, 2021

cmsbuild mentioned this pull request Apr 30, 2021

Pixel local reco: reduce code duplication between SiPixelDigiErrorsFromSoA and SiPixelRawToDigi #33526

Merged

slava77 mentioned this pull request Jun 18, 2021

review data-specific modifications for RECO related to HCAL noise flagging #27767

Closed

abdoulline mentioned this pull request Jun 29, 2021

HCAL: move data-specific SeverityLevel customization(s) to Reconstruction_Data_cff.py #34282

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adopt HFSignalAsymmetry flag in HF PFRecHit cleaning #33520

Adopt HFSignalAsymmetry flag in HF PFRecHit cleaning #33520

hatakeyamak commented Apr 25, 2021 •

edited

Loading

cmsbuild commented Apr 25, 2021

cmsbuild commented Apr 25, 2021

perrotta commented Apr 26, 2021

silviodonato commented Apr 26, 2021

cmsbuild commented Apr 26, 2021

slava77 commented Apr 27, 2021

abdoulline commented Apr 28, 2021

slava77 commented Apr 28, 2021

hatakeyamak commented Apr 28, 2021 •

edited

Loading

abdoulline commented Apr 28, 2021

slava77 commented Apr 28, 2021

abdoulline commented Apr 28, 2021

slava77 commented Apr 28, 2021

cmsbuild commented Apr 29, 2021

cmsbuild commented Apr 29, 2021

slava77 commented Apr 29, 2021

cmsbuild commented Apr 29, 2021

abdoulline commented Apr 29, 2021

slava77 commented Apr 30, 2021

silviodonato commented Apr 30, 2021

cmsbuild commented Apr 30, 2021

Adopt HFSignalAsymmetry flag in HF PFRecHit cleaning #33520

Adopt HFSignalAsymmetry flag in HF PFRecHit cleaning #33520

Conversation

hatakeyamak commented Apr 25, 2021 • edited Loading

PR description:

PR validation:

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

cmsbuild commented Apr 25, 2021

cmsbuild commented Apr 25, 2021

perrotta commented Apr 26, 2021

silviodonato commented Apr 26, 2021

cmsbuild commented Apr 26, 2021

Unit Tests

Comparison Summary

slava77 commented Apr 27, 2021

abdoulline commented Apr 28, 2021

slava77 commented Apr 28, 2021

hatakeyamak commented Apr 28, 2021 • edited Loading

abdoulline commented Apr 28, 2021

slava77 commented Apr 28, 2021

abdoulline commented Apr 28, 2021

slava77 commented Apr 28, 2021

cmsbuild commented Apr 29, 2021

cmsbuild commented Apr 29, 2021

slava77 commented Apr 29, 2021

cmsbuild commented Apr 29, 2021

Comparison Summary

abdoulline commented Apr 29, 2021

slava77 commented Apr 30, 2021

silviodonato commented Apr 30, 2021

cmsbuild commented Apr 30, 2021

hatakeyamak commented Apr 25, 2021 •

edited

Loading

hatakeyamak commented Apr 28, 2021 •

edited

Loading