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

Adaptation to enable the use of a new MVAIsolation training #21022

Merged
merged 19 commits into from Nov 3, 2017

Conversation

roger-wolf
Copy link
Contributor

Dear Slava and Andrea,

this is a PR that enables the use of a new training adapted to the increase of the pT cut required for PFPhotons used for input variables for the tauID MVA, isolation sums and tau reconstruction in general, as included in our last PR (#20900).

All changes are only on the python configuration level. The anticipated changes in performance have been studied and documented here (*). The new training is based on the Summer17 MC samples and will be referred to as 2017v1.

We hope for an integration still into 94X to have the new training included in the upcoming new MC production campaign. Standard tests show no differences beyond those that we expect to see. For more details and confirmation I refer to @mbluj, @steggema and @isobelojalvo

Cheers,
Roger

(*)
https://indico.cern.ch/event/675184/contributions/2766837/attachments/1546406/2427371/mbluj_TauId_GammaPtGt1GeV_24Oct2017.pdf
https://indico.cern.ch/event/675184/contributions/2767859/attachments/1547023/2428479/mbluj_TauId_80XLegacyMiniAOD_24Oct2017.pdf

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-21022/1649

@cmsbuild
Copy link
Contributor

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

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 you are the release manager for this.

cms-bot commands are listed here

@perrotta
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 26, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/23991/console Started: 2017/10/26 07:19

@cmsbuild
Copy link
Contributor

-1

Tested at: 00e4b6b

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
00e4b6b
56e6674
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-21022/23991/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-21022/23991/git-merge-result

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

I found follow errors while testing this PR

Failed tests: UnitTests AddOn

  • Unit Tests:

I found errors in the following unit tests:

---> test runtestPhysicsToolsPatAlgos had ERRORS

  • AddOn:

I found errors in the following addon tests:

cmsRun /build/cmsbld/jenkins-workarea/workspace/ib-any-integration/CMSSW_9_4_X_2017-10-25-2300/src/PhysicsTools/PatAlgos/test/IntegrationTest_cfg.py : FAILED - time: date Thu Oct 26 08:29:57 2017-date Thu Oct 26 08:29:18 2017 s - exit: 17920

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
00e4b6b
56e6674
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-21022/23991/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-21022/23991/git-merge-result

@cmsbuild
Copy link
Contributor

Comparison job queued.

@perrotta
Copy link
Contributor

Errors are related: @roger-wolf please fix

@mbluj
Copy link
Contributor

mbluj commented Oct 26, 2017

The error is related to a work-flow where MiniAOD is produced from existing AOD, i.e. AOD with is not rerun, which does not contain a new VVLoose WP of MVAIso (added on request of SUSY and EXO). It was checked that it works with the full production chain. How it can be fixed to satisfy for unit tests?

@slava77
Copy link
Contributor

slava77 commented Oct 26, 2017

We hope for an integration still into 94X to have the new training included in the upcoming new MC production campaign.

94X is now closed for new features.
If this is required in 94X, POG validators can invalidate 940pre3 and then we can have this update included.
I expect that some time today this PR is auto-advanced to 10X release and we will eventually need a backport.

@smuzaffar smuzaffar removed this from the CMSSW_9_4_X milestone Oct 26, 2017
@cmsbuild
Copy link
Contributor

Pull request #21022 was updated. @perrotta, @cmsbuild, @monttj, @slava77 can you please check and sign again.

@mbluj
Copy link
Contributor

mbluj commented Oct 30, 2017 via email

@perrotta
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 30, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/24042/console Started: 2017/10/30 10:23

@cmsbuild
Copy link
Contributor

@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-21022/24042/summary.html

There are some workflows for which there are errors in the baseline:
10024.0 step 5
The results for the comparisons for these workflows could be incomplete
This means most likely that the IB is having errors in the relvals.The error does NOT come from this pull request

Comparison Summary:

  • No significant changes to the logs found
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 15 differences found in the comparisons
  • DQMHistoTests: Total files compared: 25
  • DQMHistoTests: Total histograms compared: 2621497
  • DQMHistoTests: Total failures: 1652
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2619681
  • DQMHistoTests: Total skipped: 164
  • DQMHistoTests: Total Missing objects: 0
  • Checked 102 log files, 10 edm output root files, 25 DQM output files

@perrotta
Copy link
Contributor

+1

  • Training update results in modified shapes for the distributions of the tau discriminator by isolation, and this further propagates in differences in DQM distributions for all tau isolation working points
  • The results had been validated by the tau POG, as from the plots attached in the main description of this PR
  • A flag has been added to the PATTauProducer which prevents, if enabled, code crashing in case a given tau discriminator is not present in the event. This is not enabled by default (i.e. the steering configuration must correspond identically to what in the event), but it is in the addOn tests which read on not-yet updated AOD files, thus preventing them from crashing.

@perrotta
Copy link
Contributor

@mbluj @roger-wolf : this is submitted to the master, which now corresponds to CMSSW_10_0_X.
If you need it backported to 94X (previous agreemet of the release manager, PC, etc. because of the modified reco content) you must provide a backport PR.

@davidlange6
Copy link
Contributor

merge

@davidlange6
Copy link
Contributor

(but we need to finalize 80x and 94x AOD support)

@cmsbuild cmsbuild merged commit 5fbe472 into cms-sw:master Nov 3, 2017
@Dr15Jones
Copy link
Contributor

@perrotta
Copy link
Contributor

perrotta commented Nov 6, 2017

@roger-wolf @mbluj

Unit tests are broken in

@@ -315,6 +316,11 @@ void PATTauProducer::produce(edm::Event & iEvent, const edm::EventSetup & iSetup
edm::Handle<reco::PFTauDiscriminator> pfTauIdDiscr;
iEvent.getByToken(pfTauIDTokens_[i], pfTauIdDiscr);

if(skipMissingTauID_ && !pfTauIdDiscr.isValid()){
edm::LogWarning("DataSource") << "Tau discriminator '" << tauIDSrcs_[i].first
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be made a LogInfo?
The warning shows up for every tau and just pollutes the logs.
A more informative option that will still be printed is to accumulate the number of dropped IDs and print only the first time it happens.

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