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.

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

New trainings for mvaTauID + one backport from CMSSW_10_0_X #21977

Merged

Conversation

roger-wolf
Copy link
Contributor

Dear Slava, Andrea,

this PR consists of a modification of the miniAOD sequences, adding new mvaTauID discriminants that have been trained with 2017 MCv1 (tauIdMVAIsoDBoldDMwLT2017). Modified files are:

  • RecoTauTag/Configuration/python/loadRecoTauTagMVAsFromPrepDB_cfi.py
  • RecoTauTag/Configuration/python/updateHPSPFTaus.py
  • PhysicsTools/PatAlgos/python/slimming/miniAOD_tools.py

This modification impacts both the standard and legacy80X workflows via an updated makePatTausTask.

In addition the protection to prevent the PATTauProducer from asserting when encountering requests for tauID WP that are not available on existing AOD, has been backported from 100X (*). Changed files are:

  • PhysicsTools/PatAlgos/plugins/PATTauProducer.cc
  • PhysicsTools/PatAlgos/plugins/PATTauProducer.h
  • PhysicsTools/PatAlgos/python/producersLayer1/tauProducer_cfi.py

The development has been made by @mbluj.

The changes wrt to status quo are in the mvaTauID output only. Performance comparisons and more detailed comments can be checked from here (**)

Please consider this PR for reminiAODv2.

Cheers,
Roger

(*)
#21186

(**)
cms-tau-pog#71

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @roger-wolf (Roger Wolf) for CMSSW_9_4_X.

It involves the following packages:

PhysicsTools/PatAlgos
RecoTauTag/Configuration

@perrotta, @cmsbuild, @monttj, @slava77 can you please review it and eventually sign? Thanks.
@TaiSakuma, @gouskos, @imarches, @ahinzmann, @acaudron, @mmarionncern, @rappoccio, @jdamgov, @jdolen, @nhanvtran, @gpetruc, @gkasieczka, @schoef, @ferencek, @mverzett, @mariadalfonso, @pvmulder, @seemasharmafnal, @JyothsnaKomaragiri 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

@mbluj mbluj mentioned this pull request Jan 26, 2018
15 tasks
@slava77
Copy link
Contributor

slava77 commented Jan 26, 2018

@roger-wolf
is everything introduced in this PR already available in the master branch?

@slava77
Copy link
Contributor

slava77 commented Jan 26, 2018

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 26, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/25653/console Started: 2018/01/26 20:24

@@ -0,0 +1,53 @@
import FWCore.ParameterSet.Config as cms
Copy link
Contributor

Choose a reason for hiding this comment

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

the name of this file requires some disambiguation.
There is also updateHPSPFTaus_cff with completely different content.

perhaps customiseHPSPFTausForPAT ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've followed your proposals.
Cheers,
Roger

process.hpsPFTauDiscriminationByVVTightIsolationMVArun2v1DBoldDMwLTMVAIsoFor94XMiniv2.mapping[0].cut = cms.string("RecoTauTag_tauIdMVAIsoDBoldDMwLT2017v1_WPEff40")
#create a new task and put the moduled therin
process.hpsPFTauMVAIsoFor94XMiniv2Task = cms.Task()
process.hpsPFTauMVAIsoFor94XMiniv2Task.add(process.hpsPFTauDiscriminationByIsolationMVArun2v1DBoldDMwLTrawMVAIsoFor94XMiniv2)
Copy link
Contributor

Choose a reason for hiding this comment

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

it would be less verbose to simply pass all taggers in the Task constructor arguments.

@roger-wolf
Copy link
Contributor Author

Hi Slava,

yes, everything that is commited here is also part of the master branch, with the exception of the 94X legacy processing specific parts of course. For more info I refer to @mbluj . I'll up date according to your comments tomorrow, as soon as I get to it.

Cheers,
Roger

@slava77
Copy link
Contributor

slava77 commented Jan 26, 2018

After some thought I think that we should make a commitment to support miniAOD processing using inputs from 94X Fall17 RECO/AOD production campaign.
So, this means an introduction of a modifier similar to run2_miniAOD_80XLegacy.
I propose to call it run2_miniAOD_94XFall17.
Since tau is currently the only customer, Roger, may I ask that you create this modifier.
Given this modifier we can then follow this:

  • in master:
    • create run2_miniAOD_94XFall17
    • define hpsPFTauIsolationMVArun2v1DBoldDMwLTTask in HPSPFTaus_cff.py with the 8 needed modules
      • use this task in HPSPFTaus_cff.py hpsPFTauMVAIsolation2Task
    • append hpsPFTauMVAIsoTask to makePatTausTask in miniAOD_tools via run2_miniAOD_94XFall17
    • .. process.patTaus.tauIDSources remains unchanged
    • .. modules entering hpsPFTauMVAIsoTask remain unchanged
  • in 94X (almost the same, but with minor modifications)
    • create run2_miniAOD_94XFall17
    • define hpsPFTauIsolationMVArun2v1DBoldDMwLTTask in in HPSPFTaus_cff.py with 7 needed modules
      • add hpsPFTauDiscriminationByVVLooseIsolationMVArun2v1DBoldDMwLT only for this era
      • use this task in HPSPFTaus_cff.py hpsPFTauMVAIsolation2Task
    • append hpsPFTauMVAIsoTask to makePatTausTask in miniAOD_tools via run2_miniAOD_94XFall17
    • process.patTaus.tauIDSources remains unchanged
    • modules entering hpsPFTauMVAIsoTask are modified via run2_miniAOD_94XFall17 to pick up the right payload values

let me know if this is clear enough or if we should discuss more.

@cmsbuild
Copy link
Contributor

Pull request #21977 was updated. @perrotta, @monttj, @cmsbuild, @franzoni, @slava77, @fabiocos, @davidlange6 can you please check and sign again.

@slava77
Copy link
Contributor

slava77 commented Feb 21, 2018

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 21, 2018

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

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 117 differences found in the comparisons
  • DQMHistoTests: Total files compared: 26
  • DQMHistoTests: Total histograms compared: 2718773
  • DQMHistoTests: Total failures: 107
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2718504
  • DQMHistoTests: Total skipped: 162
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0 KiB( 0 files compared)
  • Checked 107 log files, 9 edm output root files, 26 DQM output files

@roger-wolf
Copy link
Contributor Author

Dear colleagues,

let me know whethere there is anything else pending to get this PR signed.

Cheers,
Roger

@slava77
Copy link
Contributor

slava77 commented Feb 27, 2018

+1

for #21977 e30e6ef

@slava77
Copy link
Contributor

slava77 commented Mar 5, 2018

@fabiocos
I wanted to check if this can be merged in 94MAODX.
Please clarify.
Thank you.

@fabiocos
Copy link
Contributor

fabiocos commented Mar 5, 2018

+operations

@fabiocos
Copy link
Contributor

fabiocos commented Mar 5, 2018

+1

@fabiocos
Copy link
Contributor

fabiocos commented Mar 5, 2018

merge

@cmsbuild cmsbuild merged commit 7c9aed8 into cms-sw:CMSSW_9_4_MAOD_X Mar 5, 2018
@gpetruc
Copy link
Contributor

gpetruc commented Mar 19, 2018

Do I understand correctly that this PR changes the MVArun2v1DBoldDMwLT set of discriminators, rather than adding a new set?

@slava77
Copy link
Contributor

slava77 commented Mar 19, 2018

Do I understand correctly that this PR changes the MVArun2v1DBoldDMwLT set of discriminators, rather than adding a new set?

One VVL discriminator is added; the rest are indeed modified.

@mbluj
Copy link
Contributor

mbluj commented Mar 19, 2018 via email

@mbluj mbluj deleted the CMSSW_9_4_X_tau-pog_newTauID-MCv1 branch October 10, 2023 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants