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

clean up of old tau discriminators and integration of new ones #13272

Merged
merged 16 commits into from Feb 18, 2016

Conversation

roger-wolf
Copy link
Contributor

The pull request is build upon CMSSW_8_0_X_2016-02-09-1100. The modifications that have been made is limited to five files in

  • RecoTauTag/Configuration (definition of even content)
  • RecoTauTag/RecoTau (main sequence)
  • Validation/RecoTau (validation configuration)
  • PhysicsTools/PatAlgos (high level analysis configuration)
    We've checked that the pat configuration runs in unscheduled mode with all RecoTauTagRECO in the outputCommands, i.e. all discriminators have been build and made persostent starting from relval RECO. The limited set of matrix tests (runTheMatrix.py -l limited -i all) passed w/o failures. The usual validation plot that we checked show unsuspicious behaviour (checked by TauPOg ynd ID conveners). We would like this request still to go into CMSSW_8_0_0_pre6 to be built from Monday on if possible.
    Thanx a lot!

…from branch 80xReMiniAOD-RemoveOldDisc to here: the following three files should be more or less clear.
…from branch 80xReMiniAOD-RemoveOldDisc to here: in the following two files I commented several lines which where present in the latest IB of 02.09.2016 -- 11:00 but not in the indicated developments branch. Please cross check that these lines can be deleted.
…ill at least slope through a solitary instance of creating tau discriminators from RECO on. I'd like to use this as a lightweighted test that things are OK w/o running this full matrix script all the time. Within the next days this might be extended and potentially moved into some RecoTau place.
…een light from convenres during the day). PileupWeighted discriminatirs remain in commented for the moment as they might remain of interest for validation purposes.
…tContent to satisfy matrx test 1000.0 (see comment to corresponding lines in RecoTauTag_EventContent_cff)
…ontinued. Note a renaming: MVA6raw --> MVA6Raw
@cmsbuild
Copy link
Contributor

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

It involves the following packages:

PhysicsTools/PatAlgos
RecoTauTag/Configuration
Validation/RecoTau

@cvuosalo, @monttj, @cmsbuild, @deguio, @slava77, @vadler, @vanbesien, @davidlange6 can you please review it and eventually sign? Thanks.
@TaiSakuma, @imarches, @ahinzmann, @acaudron, @mmarionncern, @rappoccio, @jdolen, @nhanvtran, @schoef, @ferencek, @gpetruc, @mariadalfonso, @pvmulder this is something you requested to watch as well.
@slava77, @Degano, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

byLoosePileupWeightedIsolation3Hits = cms.InputTag("hpsPFTauDiscriminationByLoosePileupWeightedIsolation3Hits"),
byMediumPileupWeightedIsolation3Hits = cms.InputTag("hpsPFTauDiscriminationByMediumPileupWeightedIsolation3Hits"),
byTightPileupWeightedIsolation3Hits = cms.InputTag("hpsPFTauDiscriminationByTightPileupWeightedIsolation3Hits"),
#byLoosePileupWeightedIsolation3Hits = cms.InputTag("hpsPFTauDiscriminationByLoosePileupWeightedIsolation3Hits"),
Copy link
Contributor

Choose a reason for hiding this comment

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

why are these commented out and not completely removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed. I've removed the commented lines. For explanation: there was a request by TauID conveners to keep these discriminators accessible for validation purposes. But validation is not run on PAT therefore your argument applies.

@slava77
Copy link
Contributor

slava77 commented Feb 13, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/11195/console

@cmsbuild
Copy link
Contributor

-1

Tested at: 231e24b
I found errors in the following unit tests:

---> test runtestPhysicsToolsPatAlgos had ERRORS

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

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Pull request #13272 was updated. @cvuosalo, @monttj, @cmsbuild, @deguio, @slava77, @vadler, @vanbesien, @davidlange6 can you please check and sign again.

@roger-wolf
Copy link
Contributor Author

I've updated the relval files for PAT now from 760 to 800pre6, which should fix this problem. I did not touch the reval section for filesRelValTTbarPileUpFastSimGENSIMDIGIRECO, which contains a comment I can not judge upon and not the SingleMuon Relval section either. Since AODSIM relval files do not exist (anymore?) I'm loading GEN-SIM-RECO instead also for the AODSIM string. On the way I've commited another minor fix.

@slava77
Copy link
Contributor

slava77 commented Feb 15, 2016

@hengne
are the AODSIM relval outputs expected to come soon or were they not included in the 800pre6?

@cvuosalo
Copy link
Contributor

@cmsbuild please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/11237/console

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@cvuosalo
Copy link
Contributor

Jenkins results show a decrease in tau L1 global efficiencies. Is this change expected? Here's an example from workflow 25.0_TTbar.
eff2

@cvuosalo
Copy link
Contributor

Here are more examples of changes from a test of workflow 25202.0_TTbar_13 with 70 events against baseline CMSSW_8_0_X_2016-02-09-1100.

Here are more efficiency drops:

eff25202

And here is one example among several of newly filled-in plots for tau offline HLT:

l2taueff

@cvuosalo
Copy link
Contributor

As might be expected, an extended test shows the deletion of the obsolete discriminators shrinks Reco event content size and reduces CPU time by a few percent. Details to follow once the questions above are resolved.

@roger-wolf
Copy link
Contributor Author

I've cross checked with the responsible persons for tau DQM and the tau POG and sub-group conveners. This looks like a plotting issue in the corresponding DQM package (https://github.com/cms-tau-pog/cmssw/tree/CMSSW_8_0_X_tau-pog/HLTriggerOffline/Tau/python/Validation) that currently is under investigation. Conveners are aware of the situation and propose to proceed. We'll keep the change in the validation plot in mind and add a fix asa available possibly at some later stage, if this is OK with you.

@cvuosalo
Copy link
Contributor

+1

For #13272 8bf3eef

Removing obsolete tau discriminators and integrating a few new ones.

The code changes are satisfactory. Jenkins tests against baseline CMSSW_8_0_X_2016-02-15-1100 show some minor differences discussed above, which will be investigated and fixed, if necessary, in a subsequent PR. An extended test of workflow 25202.0_TTbar_13 with 70 events against baseline CMSSW_8_0_X_2016-02-09-1100 shows similar differences. Reco event content size decreases by 0.2% from the loss of 58 deleted discriminators, which is partly offset by 3 newly added discriminators. Mini-AOD increases in size by 0.6% due to more tau content. CPU time decreases by about 2.3%, partly due to the eliminated discriminators. Memory usage shrinks marginally.
Here are the details about the Mini-AOD size increase:

-----------------------------------------------------------------
   or, B         new, B      delta, B   delta, %   deltaJ, %    branch 
-----------------------------------------------------------------

   1935.2 ->      2132.2        197      9.7   0.23     patTaus_slimmedTausBoosted__RECO.
   2215.7 ->      2516.9        301     12.7   0.36     patTaus_slimmedTaus__RECO.
-------------------------------------------------------------
    84181 ->       84679        498             0.6     ALL BRANCHES

@deguio
Copy link
Contributor

deguio commented Feb 18, 2016

+1

davidlange6 added a commit that referenced this pull request Feb 18, 2016
clean up of old tau discriminators and integration of new ones
@davidlange6 davidlange6 merged commit 4f9f6ec into cms-sw:CMSSW_8_0_X Feb 18, 2016
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

7 participants