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

Update of DeepTauID to ver. 2017v2 (backport for UL) #27146

Conversation

mbluj
Copy link
Contributor

@mbluj mbluj commented Jun 7, 2019

PR description:

This PR updates DeepTauID to version 2017v2. The new version, thanks to revised structure of used DNN, fixes issues of version 2017v1 that is a size of training files and memory consumption are greatly reduced. Possibility to run previous version is maintained.

DeepTauID (2017v2) is added to the tau sequence in the standard MiniAOD workflow.

Performance with 1k ttbar events:

In addition the new v2 training overperforms (physics-result-wise) old v1 one as well as other TauIDs, details in slides:

Note 1: this PR is backport of #26796 to 10_6_X for UL processing
Note 2: this PR is completed by cms-data/RecoTauTag-TrainingFiles#3 which needs to be propagated to cms-dist.

PR validation:

Validated with matrix tests; requires cms-data/RecoTauTag-TrainingFiles#3 for unit tests

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

This is port of #26796 for UL (rebased to CMSSW_10_6_X as for 7 June 2019)

@perrotta
Copy link
Contributor

+1

@fabiocos
Copy link
Contributor

@santocch @civanch could you please check this PR?

@civanch
Copy link
Contributor

civanch commented Jun 14, 2019

+1

@fabiocos
Copy link
Contributor

+1

@fabiocos
Copy link
Contributor

merge

apart for clang-formatting the backport is identical to master. The addition of the fix in #27189 will follow separately

@cmsbuild cmsbuild merged commit 6af6c5f into cms-sw:CMSSW_10_6_X Jun 14, 2019
@mbluj
Copy link
Contributor Author

mbluj commented Jun 14, 2019

Hello,
I have a question: now when this PR is merged it makes sense to store the DeepTauID also in NanoAOD - should we modify the tau table producer in NanoAOD? If so, can it go directly to CMSSW or via NanoAOD repository (technically it is quite trivial change)?

@perrotta
Copy link
Contributor

Hello,
I have a question: now when this PR is merged it makes sense to store the DeepTauID also in NanoAOD - should we modify the tau table producer in NanoAOD? If so, can it go directly to CMSSW or via NanoAOD repository (technically it is quite trivial change)?

@mbluj , the final remarks for the PR signed off in the master were (see #26796 (comment), which summarizes the discussions that went on in the github thread) "[cpu time] is probably still acceptable for miniAOD production, but a bit too much for routine nanoAOD production: some further reduction should be attempted if that ID will have to be recomputed in the nanoAOD campaigns: github issue #27119 has been opened to keep track of the future improvements in cpu performance"

@mbluj
Copy link
Contributor Author

mbluj commented Jun 14, 2019

Hello,
I have a question: now when this PR is merged it makes sense to store the DeepTauID also in NanoAOD - should we modify the tau table producer in NanoAOD? If so, can it go directly to CMSSW or via NanoAOD repository (technically it is quite trivial change)?

@mbluj , the final remarks for the PR signed off in the master were (see #26796 (comment), which summarizes the discussions that went on in the github thread) "[cpu time] is probably still acceptable for miniAOD production, but a bit too much for routine nanoAOD production: some further reduction should be attempted if that ID will have to be recomputed in the nanoAOD campaigns: github issue #27119 has been opened to keep track of the future improvements in cpu performance"

I understand this, but my question is different - I want to add DeepTau to tau table in NanoAOD, i.e. read it from Mini to write to Nano without running it at Nano step.

@perrotta
Copy link
Contributor

Hello,
I have a question: now when this PR is merged it makes sense to store the DeepTauID also in NanoAOD - should we modify the tau table producer in NanoAOD? If so, can it go directly to CMSSW or via NanoAOD repository (technically it is quite trivial change)?

@mbluj , the final remarks for the PR signed off in the master were (see #26796 (comment), which summarizes the discussions that went on in the github thread) "[cpu time] is probably still acceptable for miniAOD production, but a bit too much for routine nanoAOD production: some further reduction should be attempted if that ID will have to be recomputed in the nanoAOD campaigns: github issue #27119 has been opened to keep track of the future improvements in cpu performance"

I understand this, but my question is different - I want to add DeepTau to tau table in NanoAOD, i.e. read it from Mini to write to Nano without running it at Nano step.

Ah, ok. This will have to follow a forthcoming re-miniAOD campaign in 10_2_X, then (for which I don't know the plans).
That question should then get addressed to xpog (@fgolf,@peruzzim)

@santocch
Copy link

+1

@cmsbuild
Copy link
Contributor

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.

@peruzzim
Copy link
Contributor

Ok, I had a discussion with @steggema. It would not be nice to have the new tagger in MiniAOD UL, and not in Nano.
For this time only, we can add it directly in the cmssw repository, in the interest of a faster integration, but only if the code changes are really minimal.

It should be added in such a way that it just gets copied from MiniAOD when running without modifiers, and is deactivated when running with any of the following modifiers: run2_miniAOD_80XLegacy, run2_nanoAOD_94X2016, run2_nanoAOD_94XMiniAODv1, run2_nanoAOD_94XMiniAODv2, run2_nanoAOD_102Xv1.

You can add it to the tau table, and then do something like:
(run2_miniAOD_80XLegacy | run2_nanoAOD_94X2016 | run2_nanoAOD_94XMiniAODv1 | run2_nanoAOD_94XMiniAODv2 | run2_nanoAOD_102Xv1).toModify(tauTable.variables, newID = None)
to avoid trying to store it when you run on older MiniAOD.

I hope you realize how late this comes...

@mbluj
Copy link
Contributor Author

mbluj commented Jun 17, 2019

Ok, I had a discussion with @steggema. It would not be nice to have the new tagger in MiniAOD UL, and not in Nano.
For this time only, we can add it directly in the cmssw repository, in the interest of a faster integration, but only if the code changes are really minimal.

Yes, sure thing.

It should be added in such a way that it just gets copied from MiniAOD when running without modifiers, and is deactivated when running with any of the following modifiers: run2_miniAOD_80XLegacy, run2_nanoAOD_94X2016, run2_nanoAOD_94XMiniAODv1, run2_nanoAOD_94XMiniAODv2, run2_nanoAOD_102Xv1.

OK, will be done.

You can add it to the tau table, and then do something like:
(run2_miniAOD_80XLegacy | run2_nanoAOD_94X2016 | run2_nanoAOD_94XMiniAODv1 | run2_nanoAOD_94XMiniAODv2 | run2_nanoAOD_102Xv1).toModify(tauTable.variables, newID = None)
to avoid trying to store it when you run on older MiniAOD.

OK, thank you for this hint

I hope you realize how late this comes...

@fabiocos
Copy link
Contributor

@mbluj @peruzzim we are discussing of an update to nanoAOD targeting 10_6_X . Let's discuss at next ORP meeting, but this seems late for CMSSW_10_6_1, if we do not want to delay it further

@mbluj
Copy link
Contributor Author

mbluj commented Jun 17, 2019

@mbluj @peruzzim we are discussing of an update to nanoAOD targeting 10_6_X . Let's discuss at next ORP meeting, but this seems late for CMSSW_10_6_1, if we do not want to delay it further

I think to be ready with a PR today or early tomorrow - it is a small addition. But sure, final decision in your hands.

@mbluj mbluj deleted the CMSSW_10_6_X_tau-pog_DeepTau2017v2_forUL branch October 10, 2023 10:06
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