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

Backport of access to tauID and electron discrimination from miniAOD #17292

Conversation

roger-wolf
Copy link
Contributor

Dear colleagues,
this is a 1:1 backport of the work we have invested last autumn to provide access to tau-ID and electron discrimination on miniAOD in 81X. The relevant PRs have been:

By that time we had no chance to get these developments backported in time to the 80X release series and had announced that we would like to do this once we are set. We have now prepared the first case where tau-ID and electron-discrimination have indeed been produced and can be applied to analyses based only on miniAOD information! This has been provided for the upcoming analyses of this year. It is consequently done in 80X.

The backport introduces necessary additional contents in a few tau-specific DataFormats. No existing contents or members are changed or removed. In standard workflows there is no access to the newly added data members, so the backport does not change standard reconstruction behaviour. It only allows that existing training files may also be accessed from miniAOD in addition to the unchanged standard access from AOD. This project is of great importance for the flexibility of tau analyses in 2016.

Since only information is added we do not expect any change of the default reconstruction behaviour (see discussion of 81X pull requests). We have made the standard tests that we usually do in preparation of our pull requests to verify that this is the case. The additional features have also been successfully tested with unit-tests that had been introduced by the time of the original pull requests to 81X (and backported accordingly). In addition the whole infrastructure has already been used for the production and extensive validation of new MVA weight files, and their application. A detailed report is planed by Alexander Nehrkorn for the TauPOG meeting during the upcoming CMS week.

A small set of modifications on top of this pull request (which is a plain backport as said) would follow to give official access to the new weight files, most probably also related to a new Global Tag request. We would highly appreciate if these modifications could make it into a new 80X analysis release if possible. I'm happy to give a report or just to drop a few words of further clarification during the RECO/AT meeting tomorrow. You can put me on the agenda and let me know in case.

Cheers,
Roger

@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:

DataFormats/PatCandidates
PhysicsTools/PatAlgos
RecoTauTag/Configuration
RecoTauTag/RecoTau

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

cms-bot commands are listed here #13028

@slava77
Copy link
Contributor

slava77 commented Jan 26, 2017

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 26, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/17454/console Started: 2017/01/26 15:15

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Feb 3, 2017

@roger-wolf
you mention #15758 in the PR description. This is an 80X PR.
I'm guessing that you had in mind #15778

@roger-wolf
Copy link
Contributor Author

Hi Slava,

you are right -- #15758 is already a backport. So I must have mixed this in by incidence. Please note though that we would like to add one single variable (phiAtEcalEntrance_) to TauPFEssentials.h in PatCandidates if possible. This variable was missing for the backport of #15758 and the fix did not appear in time by that time. Since this is only an additional variable in the PAT event contents that is default filled 0 unless the new tauID sequence is run I hope that adding this member is not impossible.

Cheers,
Roger

@roger-wolf
Copy link
Contributor Author

Hi all,

all tests passed all questions answered as far as there were any. Any reasons not to sign this off so that we could continue with a few more commits on top of this?

Thanx a lot!

Cheers,
Roger

@slava77
Copy link
Contributor

slava77 commented Feb 9, 2017

Hi Roger,
I haven't checked the backport yet. That's why it's not signed.

The PR description states "this is a 1:1 backport".
I made a quick attempt to add up to the +3,083 lines in this PR and was not successful:

 Extension of miniAOD event content to be able to run tau mva isolation discriminator on miniAOD input #15731 
https://github.com/cms-sw/cmssw/pull/15731	in CMSSW_8_1_X
+81 −43	 in 8
 Extension of PAT TauPFEssential event content to be able to run anti-electron discriminators in miniAOD input #15747 
https://github.com/cms-sw/cmssw/pull/15747	in CMSSW_8_1_X
+116 −41	 in 5

 Extension of miniAOD event content to be able to run tau mva isolation discriminator on miniAOD input #15758 
https://github.com/cms-sw/cmssw/pull/15758	in CMSSW_8_0_X from Sep 8, 2016 ??? 
+145 −22	 in 6
.. this does not belong in here as a reference

 Adding missing variable to be able to run the anti-electron discriminator on present trainings #15995 
https://github.com/cms-sw/cmssw/pull/15995	in CMSSW_8_1_X
+23 −10	in 7
 Extension of infrastructure to readout tau discriminators from miniAOD to anti-electron discriminator #16030 
https://github.com/cms-sw/cmssw/pull/16030	in CMSSW_8_1_X
+607 −24	in 6

.. this clearly doesn't add up
.. there was also
 Infrastructure to create the actual ValueMap pat::Tau-discriminator for tau isolation and ID #15778 
https://github.com/cms-sw/cmssw/pull/15778	in CMSSW_8_1_X
+1,775 −225	in 19
.. 81 + 116 +23+607+1775 = 2602 is still smaller than 3083 in this 80X PR

please check and update the list of already merged 81X/90X PRs in a way that it's clear this is actually a 1:1 backport.

If you plan additional commits on top of this which will be a clear backports of features already in 80X/90X, feel free to add them here

@roger-wolf
Copy link
Contributor Author

roger-wolf commented Feb 9, 2017

Hm,

I actually made the backport based in the PR's to 81X -- Ok I'll re-check and let you know asap.

NB:
since you started the review already (many thanx for the care that you spent on it!) please let me know, whether you would still like me to match the files with the 81X/90X files to indicate that this is a backport.

Cheers,
Roger

@@ -0,0 +1,5 @@
/store/relval/CMSSW_8_1_0_pre11/RelValQCD_Pt_80_120_13/GEN-SIM-RECO/81X_mcRun2_asymptotic_Candidate_2016_08_30_11_31_55-v1/00000/24F576D4-4D74-E611-8898-0025905A48BA.root
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems inappropriate for 80X.
Please check and update to something based on 80X

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These files are only used for our internal tests. They are anyhow obsolete, once the validation samples are deleted form relval at CERN. But I've added some more intuitive dummy files for the next commit.

mva_(0),
category_output_()
{
mva_ = new AntiElectronIDMVA6(cfg);
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use a unique_ptr here as it was introduced in #16030

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok -- done for the next commit.

std::string moduleLabel_;

AntiElectronIDMVA6* mva_;
float* mvaInput_;
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this needed for here? it looks unused

Copy link
Contributor Author

Choose a reason for hiding this comment

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

has been removed. This is left over garbage that had been collected in 81X and also now in 80X.

@cmsbuild
Copy link
Contributor

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

@slava77
Copy link
Contributor

slava77 commented Feb 24, 2017

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 24, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/17952/console Started: 2017/02/24 16:26

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Mar 1, 2017

+1

for #17292 f212de7

Local tests confirm no change in RECO and changes in patTau output products.
I'm putting this on hold, to be not merged until the rereco release is to be built.

@slava77
Copy link
Contributor

slava77 commented Mar 1, 2017

hold

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 1, 2017

Pull request has been put on hold by @slava77
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

@slava77
Copy link
Contributor

slava77 commented Apr 6, 2017

@davidlange6
why was this merged?
This is nominally for miniAOD v2

@davidlange6
Copy link
Contributor

ok, i didn't understand that - should I revert?

@slava77
Copy link
Contributor

slava77 commented Apr 6, 2017 via email

@roger-wolf roger-wolf deleted the CMSSW_8_0_X_tau-pog_miniAOD-backport branch November 17, 2017 10:01
@roger-wolf roger-wolf restored the CMSSW_8_0_X_tau-pog_miniAOD-backport branch November 23, 2017 13:53
@mbluj mbluj deleted the CMSSW_8_0_X_tau-pog_miniAOD-backport branch October 10, 2023 10:15
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

4 participants