-
Notifications
You must be signed in to change notification settings - Fork 10
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 MVA-based Tau-Ids #108
Conversation
(cherry picked from commit 75e3b6a)
which branch/release you started from? @gpetruc should we merge 944 into master? |
I have started with 944 then merged nanoAOD master like this |
@arizzi : yes, every time we update the twiki recipe to a new release we must first merge that release into nanoAOD:master so that the PRs don't contain other stuff making the rebases to master-cmsswmaster more tricky |
I changed the base back and forth so it recomputed the list of commits |
@@ -63,6 +147,7 @@ def _tauId6WPMask(pattern,doc): | |||
idAntiEle = _tauId5WPMask("againstElectron%sMVA6", doc= "Anti-electron MVA discriminator V6"), | |||
idMVAnewDM = _tauId6WPMask( "by%sIsolationMVArun2v1DBnewDMwLT", doc="IsolationMVArun2v1DBnewDMwLT ID working point"), | |||
idMVAoldDM = _tauId6WPMask( "by%sIsolationMVArun2v1DBoldDMwLT", doc="IsolationMVArun2v1DBoldDMwLT ID working point"), | |||
idMVAoldDMnew = _tauId7WPMask( "by%sIsolationMVArun2v1DBoldDMwLTNew", doc="IsolationMVArun2v1DBoldDMwLT ID working point (New)"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could 'new' become '2017' or '94X' or anything more specific? ("new" is a relative concept)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree and will correct "new"-> "2017v1". Of course at some point it will be updated to something different (2017 will become default without any suffix and other "new" will appear), but hopefully users will deal with it.
Automatic test started, see https://gitlab.cern.ch/cms-nanoAOD/nanoAOD-integration/pipelines/299840/builds |
Hi @mbluj , when you say some commits are cherry-picked from the CMSSW PR, is it from cms-sw#21977 ? |
another possibility is that the additional definition from RecoTauTag/Configuration/python/loadRecoTauTagMVAsFromPrepDB_cfi.py |
Please update |
In fact the commits are cherry-picked from other earlier PR already merged to CMSSW master (part of 100X) while the ones in cms-sw#21977 (for 94X) are also cherry-picked from the original PR. Concerning DQM, yes sure, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Automatic test report for 299840
- gitlab pipeline at https://gitlab.cern.ch/cms-nanoAOD/nanoAOD-integration/pipelines/299840/builds
- outputs at https://test-cms-nanoaod-integration.web.cern.ch/integration/test_pr_108/
Code integration
Code checks not run for this PR (no source files modified)
Please update PhysicsTools/NanoAOD/python/nanoDQM_cfi.py
: take this patch or run prepareDQM.py -d -u nano_file_mc.root
, and then if needed adjust the plot range using some human common sense.
Tests
- Long test data80X (10000 events): passed, no significant changes; dqm plots: all, diff
- Long test data80Xhip (3000 events): passed, no significant changes; dqm plots: all, diff
- Long test data94X (10000 events): passed, no significant changes; dqm plots: all, diff
- Long test mc80X (10000 events): passed, no significant changes; dqm plots: all, diff
- Long test mc94X (10000 events): passed, no significant changes; dqm plots: all, diff
- Test mc_94X: passed
- Test mc_80X: passed
- Test mc_92X: passed
- Test data_92X: passed
- Test data_80X: passed
Disk size report
Sample | kb/event | ref kb/event | diff |
---|---|---|---|
TTbar MC 94X | 1.514 | 1.509 | 0.004 ( +0.3% ) |
TTbar MC 80X | 1.561 | 1.557 | 0.003 ( +0.2% ) |
Data 80X | 0.579 | 0.577 | 0.002 ( +0.3% ) |
Data 80X, Mu Run2016E | 0.510 | 0.507 | 0.003 ( +0.7% ) |
Hello, P.S. I can send other PR and close this one if it is not possible to change the head branch of a PR. |
Dear Andrea and Giovanni, it would be very helpful if this PR can be merged soon. Thank you, Christian |
Sorry, Christian, but I think that this PR waits until final MVA training are there. It is as I think that there is not any official production of NanoAOD expected before MiniAODv2 (with non-existing CMSW_945 or newer) which in principle should consists of MVAIso 2017v1 by default, so not a real use case. But please correct me if I am wrong. |
@mbluj we need to converge this week on a version of NANOAOD to produce in chain with MiniAODV2, starting from MiniAODV2 inputs. Can you clairfy how you plan to include the latest tauID? |
It's not clear to me how we want to proceed now given that for 94X miniAOD v2 we will have only the 2017v1 ID while for 94X miniAOD v1, in principle we can have both 2017 and 2016.
Between 1 and 2, it's a matter of whether you want to force people to be aware of the new ID or not. Given that the bitmasks also change because of the new VVL WP, I would prefer option 2. Between the options 1-2 vs 3-4 boils down to whether there are ongoing analysis efforts using the old ID that should not be disrupted (I hope the answer is no) In any case, this PR should be re-opened based on [mbluj:]CMSSW_9_4_X_MVA2017v1ForNano_2 so that the commits in common with 9_4_5_cand1 will not be duplicated. Also, it should be modified to use the new |
Hello, |
@steggema we need to make an AN release this week in order to process the MiniAOD v2 immediately and create nanoaod out of those. |
@arizzi, @steggema is out this week. We have MVAIso2017v2 produced but not yet uploaded to the DB. Unfortunately it would take a few days to sort out and integrate. If it is possible to wait so it could be integrated then the Tau POG would have a preference to do that. However, if not, it is not so bad to re-run at miniAOD. I suppose it would require then a new version of nanoAOD. Apologies for not watching this thread more closely. |
@arizzi @gpetruc, where we stand with this PR? Do we have still time to try upgrade it to MVA2017v2? If yes, we will try to provide it by tomorrow early afternoon (say by about lunch time). If not we will try to finish with option 2 with a setup giving same output NanoAOD with both MiniAODv1 and v2. Implementation of the latter should be quicker than the former, hopefully by this afternoon (however not fully granted). |
Hello, |
The MVA-based Tau-IDs are going to be retrained with 94X Fall17 samples and then should be evaluated using MiniAOD Taus (skimmedTaus) and added to NanoAOD.
This is an initial implementation, based on MVA training with 92X Summer17 samples, mentioned in the issue #107. The idea is that you start revision from this initial implementation which will be updated when new MVA trainings are provided.
Note: This PR bases on CMSSW_9_4_4 which looks be newer than the master branch of NanoAOD repository. Therefore, the PR consists of a set of unrelated commits - only ones dated for 8/2/2018 cover work done here. The commits change two files:
RecoTauTag/Configuration/python/loadRecoTauTagMVAsFromPrepDB_cfi.py
with definition of MVA payloads (cherry-picked from a PR to official CMSSW), andPhysicsTools/NanoAOD/python/taus_cff.py
where new Tau-Ids are evaluated and added to the Tau tableFYI, @veelken @steggema @ohlushch