Disabled PCA inputs for deepTau v2 (10_6_X) #27882

mbluj · 2019-08-27T11:10:58Z

PR description:

Problems have been detected with dxy_PCA input variables to DeepTauID which are inconsistent between data and MC. This PR sets input values to 0 and disables the variables this way.
Issue and fix were discussed in https://indico.cern.ch/event/842796/contributions/3541592/attachments/1897386/3130770/2019-08-26_DeepTau_ID_2017v2_DY_MC_vs_Embedded_and_bug_in_definition_of_PCA_variables.pdf (see slide 7 in particular)

Backport of #27878 to 10_6_X for ultra-legacy MiniAODv2.

PR validation:

compiles & re-MiniAOD test (runTheMatrix.py -l 1325.51 -i all --ibeos)

if this PR is a backport please specify the original PR:

#27878

@swozniewski, FYI

cmsbuild · 2019-08-27T11:11:27Z

A new Pull Request was created by @mbluj for CMSSW_10_6_X.

It involves the following packages:

RecoTauTag/RecoTau

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

Backported from Disabled PCA inputs for deepTau v2 #27878

slava77 · 2019-08-27T12:36:33Z

@cmsbuild please test

cmsbuild · 2019-08-27T12:37:03Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/2218/console Started: 2019/08/27 14:37

cmsbuild · 2019-08-27T15:15:19Z

+1
Tested at: 65902ee
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f6c508/2218/summary.html

cmsbuild · 2019-08-27T15:15:23Z

Comparison job queued.

cmsbuild · 2019-08-27T17:57:35Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 111 differences found in the comparisons
DQMHistoTests: Total files compared: 33
DQMHistoTests: Total histograms compared: 3211049
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3210714
DQMHistoTests: Total skipped: 334
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 32 files compared)
Checked 137 log files, 14 edm output root files, 33 DQM output files

slava77 · 2019-08-27T21:58:10Z

Reco comparison results: 111 differences found in the comparisons

changes to physics are nominally not allowed in a production release (no-change policy).
Since this is miniAOD, there is still a chance to make this update proper in re-miniAOD.
To follow the no-change policy, this update should be made configurable and disabled by default.
Currently we do not have a special UL re-miniAOD modifier. For the time being run2_miniAOD_devel can be used.

swozniewski · 2019-08-28T07:28:48Z

In this case, is it useful to already migrate this backport with the preliminary modifier? Or should we rather keep the PR open until the proper modifier is available?

mbluj · 2019-08-28T07:46:34Z

Reco comparison results: 111 differences found in the comparisons

changes to physics are nominally not allowed in a production release (no-change policy).
Since this is miniAOD, there is still a chance to make this update proper in re-miniAOD.
To follow the no-change policy, this update should be made configurable and disabled by default.
Currently we do not have a special UL re-miniAOD modifier. For the time being run2_miniAOD_devel can be used.

mbluj · 2019-08-28T07:49:28Z

{closed by mistake, so reopened, sorry)
A question: does the comment on modifier applies only to 10_6_X version as used in a production workflow or also to versions for master (devel) and backport to 10_2_X (out of official workflows)?

slava77 · 2019-08-28T11:55:19Z

{closed by mistake, so reopened, sorry)
A question: does the comment on modifier applies only to 10_6_X version as used in a production workflow or also to versions for master (devel) and backport to 10_2_X (out of official workflows)?

it may end up in a "case by case" situation.
Let's follow up on the 10_2_X in the 10_2_X PR.

slava77 · 2019-08-28T11:57:02Z

In this case, is it useful to already migrate this backport with the preliminary modifier? Or should we rather keep the PR open until the proper modifier is available?

I suggest to use the existing modifier now. On our side, we should create an issue to keep track of this and other requests for reminiAOD so that this feature does not get lost.

slava77 · 2019-08-28T12:15:20Z

I created #27889

fabiocos · 2019-09-19T12:29:37Z

+operations

the update is coherent with the strategy agreed for the backport of this code

fabiocos · 2019-09-19T13:07:04Z

@peruzzim @santocch could you please check this backport?

peruzzim · 2019-09-19T15:47:24Z

+xpog

fabiocos · 2019-09-19T15:51:25Z

+1

fabiocos · 2019-09-19T15:51:30Z

merge

santocch · 2019-09-19T17:11:24Z

+1

cmsbuild · 2019-09-19T17:11:55Z

This pull request is fully signed and it will be integrated in one of the next CMSSW_10_6_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_11_0_X is complete. This pull request will be automatically merged.

fabiocos · 2019-09-20T07:39:52Z

@peruzzim unfortunately this does not work out of the box:

----- Begin Fatal Exception 20-Sep-2019 07:23:08 CEST-----------------------
An exception of category 'Key not found' occurred while
   [0] Processing  Event run: 1 lumi: 1 event: 2 stream: 3
   [1] Running path 'nanoAOD_step'
   [2] Calling method for module SimpleCandidateFlatTableProducer/'tauTable'
Exception Message:
pat::Tau: the ID againstElectronVTightMVA62018 can't be found in this pat::Tau.
The available IDs are: 'againstElectronLooseMVA6' 'againstElectronMVA6Raw' 'againstElectronMVA6category' 'againstElectronMediumMVA6' 'againstElectronTightMVA6' 'againstElectronVLooseMVA6' 'againstElectronVTightMVA6' 'againstMuonLoose3' 'againstMuonTight3' 'byCombinedIsolationDeltaBetaCorrRaw3Hits' 'byIsolationMVArun2v1DBdR03oldDMwLTraw' 'byIsolationMVArun2v1DBnewDMwLTraw' 'byIsolationMVArun2v1DBoldDMwLTraw' 'byIsolationMVArun2v1PWdR03oldDMwLTraw' 'byIsolationMVArun2v1PWnewDMwLTraw' 'byIsolationMVArun2v1PWoldDMwLTraw' 'byLooseCombinedIsolationDeltaBetaCorr3Hits' 'byLooseIsolationMVArun2v1DBdR03oldDMwLT' 'byLooseIsolationMVArun2v1DBnewDMwLT' 'byLooseIsolationMVArun2v1DBoldDMwLT' 'byLooseIsolationMVArun2v1PWdR03oldDMwLT' 'byLooseIsolationMVArun2v1PWnewDMwLT' 'byLooseIsolationMVArun2v1PWoldDMwLT' 'byMediumCombinedIsolationDeltaBetaCorr3Hits' 'byMediumIsolationMVArun2v1DBdR03oldDMwLT' 'byMediumIsolationMVArun2v1DBnewDMwLT' 'byMediumIsolationMVArun2v1DBoldDMwLT' 'byMediumIsolationMVArun2v1PWdR03oldDMwLT' 'byMediumIsolationMVArun2v1PWnewDMwLT' 'byMediumIsolationMVArun2v1PWoldDMwLT' 'byPhotonPtSumOutsideSignalCone' 'byTightCombinedIsolationDeltaBetaCorr3Hits' 'byTightIsolationMVArun2v1DBdR03oldDMwLT' 'byTightIsolationMVArun2v1DBnewDMwLT' 'byTightIsolationMVArun2v1DBoldDMwLT' 'byTightIsolationMVArun2v1PWdR03oldDMwLT' 'byTightIsolationMVArun2v1PWnewDMwLT' 'byTightIsolationMVArun2v1PWoldDMwLT' 'byVLooseIsolationMVArun2v1DBdR03oldDMwLT' 'byVLooseIsolationMVArun2v1DBnewDMwLT' 'byVLooseIsolationMVArun2v1DBoldDMwLT' 'byVLooseIsolationMVArun2v1PWdR03oldDMwLT' 'byVLooseIsolationMVArun2v1PWnewDMwLT' 'byVLooseIsolationMVArun2v1PWoldDMwLT' 'byVTightIsolationMVArun2v1DBdR03oldDMwLT' 'byVTightIsolationMVArun2v1DBnewDMwLT' 'byVTightIsolationMVArun2v1DBoldDMwLT' 'byVTightIsolationMVArun2v1PWdR03oldDMwLT' 'byVTightIsolationMVArun2v1PWnewDMwLT' 'byVTightIsolationMVArun2v1PWoldDMwLT' 'byVVTightIsolationMVArun2v1DBdR03oldDMwLT' 'byVVTightIsolationMVArun2v1DBnewDMwLT' 'byVVTightIsolationMVArun2v1DBoldDMwLT' 'byVVTightIsolationMVArun2v1PWdR03oldDMwLT' 'byVVTightIsolationMVArun2v1PWnewDMwLT' 'byVVTightIsolationMVArun2v1PWoldDMwLT' 'chargedIsoPtSum' 'chargedIsoPtSumdR03' 'decayModeFinding' 'decayModeFindingNewDMs' 'footprintCorrection' 'footprintCorrectiondR03' 'neutralIsoPtSum' 'neutralIsoPtSumWeight' 'neutralIsoPtSumWeightdR03' 'neutralIsoPtSumdR03' 'photonPtSumOutsideSignalCone' 'photonPtSumOutsideSignalConedR03' 'puCorrPtSum' .
----- End Fatal Exception -------------------------------------------------

in at least 3 test workflows 1329.1, 136.772 and 136.8521 . @mbluj I temporarily revert the PR waiting for a fix , so as to move forward with a build for other independent needs

mbluj · 2019-09-20T08:45:21Z

It is strange as the PR passed matrixTests and the NanoAOD workfolw has been particularly tested by myself... I will take a look ASAP.

mbluj · 2019-09-20T13:08:01Z

I checked and found that the there are conflicting era modifiers in failing workflows.
Namely, the test workflows are run with era modifiers Run2_2018 and run2_nanoAOD_102Xv1 (136.8521), or Run2_2016 and run2_miniAOD_80XLegacy (1329.1), where main era modifiers (Run2_2018, Run2_2016) are chaining modifiers enabling DeepTauID reconstruction at MiniAOD and then its reading by NanoAOD (run2_tau_ul_2016, run2_tau_ul_2016) while run2_nanoAOD_102Xv1 and run2_miniAOD_80XLegacy eras assume that DeepTauIDs (of any version) are not present.
I am trying to find a way to fix this issue.

mbluj · 2019-09-20T15:15:29Z

Problem is fixed in the most recent commit by proper ordering era modifiers in the PhysicsTools/NanoAOD/python/taus_cff.py configuration file.
Please update this PR and test.

perrotta · 2019-09-20T15:47:43Z

Problem is fixed in the most recent commit by proper ordering era modifiers in the PhysicsTools/NanoAOD/python/taus_cff.py configuration file.
Please update this PR and test.

Thank you Michal.
This PR is closed and cannot be updated.
What I think one could do is to revert the revert made by @fabiocos , and then submit another PR which only includes the extra commit (which by the way I cannot see here). Or just add such a commit to the "revert of the revert" PR.

Should the same fix also get applied to the master version of this PR?

mbluj · 2019-09-20T16:05:28Z

Problem is fixed in the most recent commit by proper ordering era modifiers in the PhysicsTools/NanoAOD/python/taus_cff.py configuration file.
Please update this PR and test.

Thank you Michal.
This PR is closed and cannot be updated.
What I think one could do is to revert the revert made by @fabiocos , and then submit another PR which only includes the extra commit (which by the way I cannot see here). Or just add such a commit to the "revert of the revert" PR.

OK, I am preparing PR to the revert branch with hope that it is what you mean here.

Should the same fix also get applied to the master version of this PR?

No, it is not needed neither for master nor for 10_2_X; the issue is related only to 10_6_X where new specific tau_ul eras have been added and then chained to master Run201x ones.

fabiocos · 2019-09-21T15:55:52Z

indeed we haven't seen any problem in master, this is due to the adjustment required by backport

Disabled PCA inputs for deepTau v2.

65902ee

cmsbuild added this to the CMSSW_10_6_X milestone Aug 27, 2019

cmsbuild added comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Aug 27, 2019

cmsbuild added tests-started and removed tests-pending labels Aug 27, 2019

cmsbuild added tests-approved and removed tests-started labels Aug 27, 2019

cmsbuild added comparison-available and removed comparison-pending labels Aug 27, 2019

mbluj closed this Aug 28, 2019

mbluj reopened this Aug 28, 2019

slava77 mentioned this pull request Aug 28, 2019

features for UL re-miniAOD in 10_6_X #27889

Closed

41 tasks

Control disabling dxy_PCA by a parameter

6b6cf55

cmsbuild added operations-approved and removed operations-pending labels Sep 19, 2019

fabiocos mentioned this pull request Sep 19, 2019

Update tau anti electron discriminators in RECO and MiniAOD #27465

Merged

cmsbuild added xpog-approved and removed xpog-pending labels Sep 19, 2019

cmsbuild added orp-approved and removed orp-pending labels Sep 19, 2019

cmsbuild merged commit 3353beb into cms-sw:CMSSW_10_6_X Sep 19, 2019

cmsbuild added analysis-approved fully-signed and removed analysis-pending pending-signatures labels Sep 19, 2019

fabiocos mentioned this pull request Sep 20, 2019

Temporarily revert "Disabled PCA inputs for deepTau v2 (10_6_X)" #28032

Merged

mbluj mentioned this pull request Sep 20, 2019

Fix order of era modifiers for DeepTau 2017v2p1 #28041

Closed

This was referenced Sep 21, 2019

Revert "Temporarily revert "Disabled PCA inputs for deepTau v2 (10_6_X)"" #28045

Closed

Restore Disabled PCA inputs for deepTau v2 (10_6_X) #28046

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disabled PCA inputs for deepTau v2 (10_6_X) #27882

Disabled PCA inputs for deepTau v2 (10_6_X) #27882

mbluj commented Aug 27, 2019

cmsbuild commented Aug 27, 2019 •

edited

Loading

slava77 commented Aug 27, 2019

cmsbuild commented Aug 27, 2019 •

edited

Loading

cmsbuild commented Aug 27, 2019

cmsbuild commented Aug 27, 2019

cmsbuild commented Aug 27, 2019

slava77 commented Aug 27, 2019

swozniewski commented Aug 28, 2019

mbluj commented Aug 28, 2019

mbluj commented Aug 28, 2019

slava77 commented Aug 28, 2019

slava77 commented Aug 28, 2019

slava77 commented Aug 28, 2019

fabiocos commented Sep 19, 2019

fabiocos commented Sep 19, 2019

peruzzim commented Sep 19, 2019

fabiocos commented Sep 19, 2019

fabiocos commented Sep 19, 2019

santocch commented Sep 19, 2019

cmsbuild commented Sep 19, 2019

fabiocos commented Sep 20, 2019

mbluj commented Sep 20, 2019

mbluj commented Sep 20, 2019

mbluj commented Sep 20, 2019 •

edited

Loading

perrotta commented Sep 20, 2019

mbluj commented Sep 20, 2019

fabiocos commented Sep 21, 2019

Disabled PCA inputs for deepTau v2 (10_6_X) #27882

Disabled PCA inputs for deepTau v2 (10_6_X) #27882

Conversation

mbluj commented Aug 27, 2019

PR description:

PR validation:

if this PR is a backport please specify the original PR:

cmsbuild commented Aug 27, 2019 • edited Loading

slava77 commented Aug 27, 2019

cmsbuild commented Aug 27, 2019 • edited Loading

cmsbuild commented Aug 27, 2019

cmsbuild commented Aug 27, 2019

cmsbuild commented Aug 27, 2019

slava77 commented Aug 27, 2019

swozniewski commented Aug 28, 2019

mbluj commented Aug 28, 2019

mbluj commented Aug 28, 2019

slava77 commented Aug 28, 2019

slava77 commented Aug 28, 2019

slava77 commented Aug 28, 2019

fabiocos commented Sep 19, 2019

fabiocos commented Sep 19, 2019

peruzzim commented Sep 19, 2019

fabiocos commented Sep 19, 2019

fabiocos commented Sep 19, 2019

santocch commented Sep 19, 2019

cmsbuild commented Sep 19, 2019

fabiocos commented Sep 20, 2019

mbluj commented Sep 20, 2019

mbluj commented Sep 20, 2019

mbluj commented Sep 20, 2019 • edited Loading

perrotta commented Sep 20, 2019

mbluj commented Sep 20, 2019

fabiocos commented Sep 21, 2019

cmsbuild commented Aug 27, 2019 •

edited

Loading

cmsbuild commented Aug 27, 2019 •

edited

Loading

mbluj commented Sep 20, 2019 •

edited

Loading