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

Adjust anti-e tauIDs to what is expected to be present in UL re-miniAOD #535

Conversation

mbluj
Copy link

@mbluj mbluj commented Aug 6, 2020

PR description:

As title of this PR says - it adjusts way in which anti-e tauIDs are retrieved to what is expected to be present in UL re-miniAOD. There are two pieces:

The changes introduced in this PR are expected be inactive with any already defined nanoAOD era, while for different eras the new anti-electron-in-deadECal tauID appears in tauTable and tau sequences are adjusted to different content of slimmedTaus in miniAOD.

PR validation:

Checked with cmsDriver-generated NanoAOD configurations that expected changes in NanoAOD configuration behave as expected for different tested eras.

@mariadalfonso
Copy link

mariadalfonso commented Aug 6, 2020

@mbluj
following a general discussion at the xPOG of jul28 we have changed a bit how we works (we are slowly dismissing cms-nano in favour of making PR directly to cms-sw).

Can you make the PR in master of cms-sw ? once that is tested we think on the back port to 10_6

@mbluj
Copy link
Author

mbluj commented Aug 6, 2020

@mbluj
following a general discussion at the xPOG of jul28 we have changed a bit how we works (we are slowly dismissing cms-nano in favour of making PR directly to cms-sw).

Can you make the PR in master of cms-sw ? once that is tested we think on the back port to 10_6

I have not been aware about this change of policy. Nevertheless, this PR backports what is already in the cms-sw for long time - this PR is backport of cms-sw#27465 (except part related to in the anti-electron-in-deadECal tauID as in #521, see discussion below). Developments in cms-sw#27465 have not been originally expected to be backported, but it was found useful in the context of UL re-MiniAOD. So how to proceed? Should I commit this PR directly to cms-sw's 10_6_X branch (for me it will be actually convenient)? If you agree I will extent cms-sw#31065.
What concerns the anti-electron-in-deadECal tauID - there is PR #521 for 10_6_X (being also a part of this PR) which is already somehow revised and not-yet-submitted PR to current master development branch. I will commit the latter timely to cms-sw. But what about #521 for 10_6_X? Can it be also directly committed to cms-sw? If so, can it be a part of extended cms-sw#31065?

@mariadalfonso
Copy link

@mbluj

MASTER:
cms-sw#27465 (entered CMSSW_11_0_0_pre7) contains those two
PhysicsTools/NanoAOD/python/taus_cff.py
PhysicsTools/NanoAOD/python/taus_updatedMVAIds_cff.py

BACKPORT
cms-sw#31065
indeed let's add those change in there are well as it's easier for everybody. After please close this PR.
we can only merge after the PR cms-sw#31063 that update the 10_6 is updated.

regarding the #521 indeed there is something we need to understand better.

(keep two releases is complicated by the fact that now you are developing objects and updating nano at the same time, and this PR is a perfect example)

@mbluj
Copy link
Author

mbluj commented Aug 6, 2020

@mbluj

MASTER:
cms-sw#27465 (entered CMSSW_11_0_0_pre7) contains those two
PhysicsTools/NanoAOD/python/taus_cff.py
PhysicsTools/NanoAOD/python/taus_updatedMVAIds_cff.py

Yes correct.

BACKPORT
cms-sw#31065
indeed let's add those change in there are well as it's easier for everybody. After please close this PR.
we can only merge after the PR cms-sw#31063 that update the 10_6 is updated.

OK, I will do it. I hope it will not produce conflicts.

regarding the #521 indeed there is something we need to understand better.

What do you exactly mean, i.e. what needs be understood? For me it will be convenient to commit a #521's counterpart for cms-sw master and include #521 as part of PR to cms-sw discussing above.

(keep two releases is complicated by the fact that now you are developing objects and updating nano at the same time, and this PR is a perfect example)

Yes it is indeed the origin of issues. The thing is that developments discussed here are not really big changes to NanoAOD, but rather some adjustments of NanoAOD content and/or sequences to what is present in MiniAOD for different eras. So it is convenient that the two go together to the same repository with one unique PR. But, it the past this way has been discouraged,

@mariadalfonso
Copy link

Specifically #521 is only for the re-miniAOD ?
do we already have one in 11_1 or 11_2 master ?

The eras should be like this:

or master:
if run2_nanoAOD_106Xv1 (+ any other old):
deal with then DeadEcal

for 10_6:
if NOT run2_miniAOD_devel:
do nothing

Beside this, also in general I have been told that the taus in the UL17 and UL18,16 are different DataFormats and that would means that I need to add the nanoAOD_106Xv2 ...

@mbluj
Copy link
Author

mbluj commented Aug 6, 2020

Specifically #521 is only for the re-miniAOD ?

Yes, exactly.

do we already have one in 11_1 or 11_2 master ?

Not yet integrated, I have just committed a PR , please check cms-sw#31077.

The eras should be like this:

or master:
if run2_nanoAOD_106Xv1 (+ any other old):
deal with then DeadEcal

Actaully, for master I add DeadEcal by default and remove for run2_nanoAOD_106Xv1 or any other older nanoAOD era.

for 10_6:
if NOT run2_miniAOD_devel:
do nothing

OK, I will do it like this. My current implementation is to add DeadEcal by default and remove for run2_nanoAOD_106Xv1 or any other older nanoAOD era which, I agree, is not fully correct. But, it is ensured that nanoAOD is run with miniAOD related era? It will be probably different than run2_miniAOD_devel at the end.

Beside this, also in general I have been told that the taus in the UL17 and UL18,16 are different DataFormats and that would means that I need to add the nanoAOD_106Xv2 ...

No, taus DataFormats are neither changed between UL17 and UL18,16 nor between them and expected UL re-miniAOD - in miniAOD which is read to build nanoAOD it is always a pat::tau collection called slimmedTaus. What changes a bit is a set of tauIDs stored within those taus:

  • UL18,16 have updated deepTauID from v2 in UL17 to v2p1; this is solved by running deepTau v2p1 for NanoAOD on top of UL17 miniAOD => no issue here;
  • UL re-MiniAOD will have deepTau v2p1 for all years, deadEcal tauID and updated anti-e MVA tauIDs. The updated updated anti-e MVA tauIDs do not require new nanoAOD era/version as it is stored in NanoAOD already for some time, what changes is that is it not run on the fly when NanoAOD is produced. Adding deadEcal tauID changes NanoAOD content (one boolean is added to tauTable) which does not require a new era explicitly as it is eanbled if era is other than any old nanoAOD era (to be changed to run2_miniAOD_devel as discussed above).

This is at least my understanding...

@mariadalfonso
Copy link

Specifically #521 is only for the re-miniAOD ?

Yes, exactly.

do we already have one in 11_1 or 11_2 master ?

Not yet integrated, I have just committed a PR , please check cms-sw#31077.

The eras should be like this:
or master:
if run2_nanoAOD_106Xv1 (+ any other old):
deal with then DeadEcal

Actaully, for master I add DeadEcal by default and remove for run2_nanoAOD_106Xv1 or any other older nanoAOD era.

indeed, what you have done in 31077 is correct.

for 10_6:
if NOT run2_miniAOD_devel:
do nothing

OK, I will do it like this. My current implementation is to add DeadEcal by default and remove for run2_nanoAOD_106Xv1 or any other older nanoAOD era which, I agree, is not fully correct. But, it is ensured that nanoAOD is run with miniAOD related era? It will be probably different than run2_miniAOD_devel at the end.

Beside this, also in general I have been told that the taus in the UL17 and UL18,16 are different DataFormats and that would means that I need to add the nanoAOD_106Xv2 ...

No, taus DataFormats are neither changed between UL17 and UL18,16 nor between them and expected UL re-miniAOD - in miniAOD which is read to build nanoAOD it is always a pat::tau collection called slimmedTaus. What changes a bit is a set of tauIDs stored within those taus:

ok, thanks for all these clarifications ingredients.

  • UL18,16 have updated deepTauID from v2 in UL17 to v2p1; this is solved by running deepTau v2p1 for NanoAOD on top of UL17 miniAOD => no issue here;
  • UL re-MiniAOD will have deepTau v2p1 for all years, deadEcal tauID and updated anti-e MVA tauIDs. The updated updated anti-e MVA tauIDs do not require new nanoAOD era/version as it is stored in NanoAOD already for some time, what changes is that is it not run on the fly when NanoAOD is produced. Adding deadEcal tauID changes NanoAOD content (one boolean is added to tauTable) which does not require a new era explicitly as it is eanbled if era is other than any old nanoAOD era (to be changed to run2_miniAOD_devel as discussed above).

So suppose we pick master to run on UL17+18 already made Mini + remini at the same time:
if UL17 ( with modified run2_nanoAOD_106Xv1) --> rerun the deepTau v2p1 , doNothing for deadEcal tauID, pick old anti-e
if UL18-16 (with modified run2_nanoAOD_106Xv2) --> pick deepTauID v2p1, doNothing for deadEcal tauID, pick old anti-e)
if (undefined modified to be used as nano for the remini) --> pick deepTauID v2p1, pick deadEcal and pick anti-e.

This is at least my understanding...

@ Peruzzi

@mbluj
Copy link
Author

mbluj commented Aug 6, 2020

So suppose we pick master to run on UL17+18 already made Mini + remini at the same time:
if UL17 ( with modified run2_nanoAOD_106Xv1) --> rerun the deepTau v2p1 , doNothing for deadEcal tauID, pick old anti-e

Yes.

if UL18-16 (with modified run2_nanoAOD_106Xv2) --> pick deepTauID v2p1, doNothing for deadEcal tauID, pick old anti-e)

Yes (assuming that 106Xv2 is a mistype and it should read 106Xv1)

if (undefined modified to be used as nano for the remini) --> pick deepTauID v2p1, pick deadEcal and pick anti-e.

Yes, for this I will use run2_miniAOD_devel as a proxy modifier within PR I am preparing to cms-sw 10_6_X. It should be changed later when a "production" modifier is defined for UL re-miniAOD and related nanoAOD (not necessarily same modifier for both mini and nano). Is it correct?

I will let you know when the PR to cms-sw 10_6_X is ready and then close this one and #521.

@mariadalfonso
Copy link

So suppose we pick master to run on UL17+18 already made Mini + remini at the same time:
if UL17 ( with modified run2_nanoAOD_106Xv1) --> rerun the deepTau v2p1 , doNothing for deadEcal tauID, pick old anti-e

Yes.

if UL18-16 (with modified run2_nanoAOD_106Xv2) --> pick deepTauID v2p1, doNothing for deadEcal tauID, pick old anti-e)

Yes (assuming that 106Xv2 is a mistype and it should read 106Xv1)

It seems that for the UL18-16 we have two options:
A) use the run2_nanoAOD_106Xv1 , execute what we will do for the UL17, and re-run anyway the deepTauID v2p1 in the nanoStep
B) define a new run2_nanoAOD_106Xv2 and pick the already calculated deepTauID-v2p1 stored in the miniAOD

you seems to prefer A), and indeed might work. I was thinking of B).
A) it's spend some redundant cpu but at least we do not introduce new modifiers.

if (undefined modified to be used as nano for the remini) --> pick deepTauID v2p1, pick deadEcal and pick anti-e.

Yes, for this I will use run2_miniAOD_devel as a proxy modifier within PR I am preparing to cms-sw 10_6_X. It should be changed later when a "production" modifier is defined for UL re-miniAOD and related nanoAOD (not necessarily same modifier for both mini and nano). Is it correct?

I will let you know when the PR to cms-sw 10_6_X is ready and then close this one and #521.

@mbluj
Copy link
Author

mbluj commented Aug 7, 2020

So suppose we pick master to run on UL17+18 already made Mini + remini at the same time:
if UL17 ( with modified run2_nanoAOD_106Xv1) --> rerun the deepTau v2p1 , doNothing for deadEcal tauID, pick old anti-e

Yes.

if UL18-16 (with modified run2_nanoAOD_106Xv2) --> pick deepTauID v2p1, doNothing for deadEcal tauID, pick old anti-e)

Yes (assuming that 106Xv2 is a mistype and it should read 106Xv1)

It seems that for the UL18-16 we have two options:
A) use the run2_nanoAOD_106Xv1 , execute what we will do for the UL17, and re-run anyway the deepTauID v2p1 in the nanoStep
B) define a new run2_nanoAOD_106Xv2 and pick the already calculated deepTauID-v2p1 stored in the miniAOD

you seems to prefer A), and indeed might work. I was thinking of B).
A) it's spend some redundant cpu but at least we do not introduce new modifiers.

Actually, run2_nanoAOD_106Xv1 is setup in that way that deepTauID v2p1 is rerun in the nanoAOD step only for UL17, while it is picked from miniAOD for UL16&18, see https://github.com/cms-sw/cmssw/blob/CMSSW_10_6_X/PhysicsTools/NanoAOD/python/nano_cff.py#L300-L308. So it means that option B is already there with run2_nanoAOD_106Xv1 and it is what I meant.

@mbluj
Copy link
Author

mbluj commented Aug 7, 2020

I will let you know when the PR to cms-sw 10_6_X is ready and then close this one and #521.

Just to let you know that cms-sw#31065 was updated to cover also the nanoAOD-related part containing adjustment to use updated anti-e tauIDs (this PR) and adding deadECal tauID (#521). Therefore, I close this PR and #521.

@mariadalfonso
Copy link

It seems that for the UL18-16 we have two options:
A) use the run2_nanoAOD_106Xv1 , execute what we will do for the UL17, and re-run anyway the deepTauID v2p1 in the nanoStep
B) define a new run2_nanoAOD_106Xv2 and pick the already calculated deepTauID-v2p1 stored in the miniAOD
you seems to prefer A), and indeed might work. I was thinking of B).
A) it's spend some redundant cpu but at least we do not introduce new modifiers.

Actually, run2_nanoAOD_106Xv1 is setup in that way that deepTauID v2p1 is rerun in the nanoAOD step only for UL17, while it is picked from miniAOD for UL16&18, see https://github.com/cms-sw/cmssw/blob/CMSSW_10_6_X/PhysicsTools/NanoAOD/python/nano_cff.py#L300-L308. So it means that option B is already there with run2_nanoAOD_106Xv1 and it is what I meant.

ok, very good: since is taken care properly with the run2_tau_ul_2016 | run2_tau_ul_2018 we do not need the run2_nanoAOD_106Xv2,

thanks for all the clarifications on this aspect

@mbluj mbluj deleted the CMSSW_10_6_X_tau-pog_updateAntiEleDiscrNano branch October 10, 2023 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants