opened
isobelojalvo
username_0
In 76X we were missing some necessary tau discriminators and had an issue with a new decay mode in the decay mode finding algorithm. The fix was applied in 76X by re-running the Taus at the miniAOD step. This pull request makes the RECO/AOD sequence the same as what is run in the miniAOD sequence in 76X (i.e. integration of needed discriminators + bugfix) and removes the 're-running' of the taus in miniAOD.
Integration of 76X Tau re-miniAOD into main 80X Sequence
issue
created
slava77
username_1
@username_0 what is the relationship between this PR and #13042 ?
It looks like the same code is modified, also, the original branch in #13402 was removed, which I guess means that #13402  should be closed.
Please clarify.
Thank you.
comment
created
isobelojalvo
username_0
Hi, The 80xReMiniAOD should be used and the 80xReMiniAODv2 had been created as a dummy for testing purposes. I should have notified AJ of this earlier! Apologies! Could you please work off this pull request?
comment
created
slava77
username_1
@cmsbuild please test
comment
created
isobelojalvo
username_0
Checking the unit tests I see they are run on the following sample:

25-Jan-2016 22:04:29 CET  Successfully opened file root://eoscms.cern.ch//eos/cms/store/relval/CMSSW_7_6_0_pre7/RelValProdTTbar_13/AODSIM/76X_mcRun2_asymptotic_v5-v1/00000/0E9A5DE8-1D71-E511-A205-00261894380D.root

The error we see is that MVA6 is not present. This is certainly true: we did not have MVA6 in 7_6_0 at MVA6. Am I missing something? @username_1 @username_2 @andrewj314
comment
created
cvuosalo
username_2
@username_0: For the RECO product of workflow 134.805_RunMET2015C, I find 64 added tau discriminators and 16 removed, for a net gain of 48 discriminators. I think we want to keep the net addition of discriminators to be less than 10. Could you please remove more obsolete and unneeded products?

I see no change in Mini-AOD event content, and I'm still checking AOD.
comment
created
cvuosalo
username_2
@username_0: The PR description does not describe the results of "fixing" the decay mode algorithm. There are numerous differences showing up for workflow 134.805_RunMET2015C. Are these differences expected? If so, please add an explanation of the expected differences to the PR description. Here are some examples:
![oldmseffeta](https://cloud.githubusercontent.com/assets/5736159/12592702/2f9afae8-c433-11e5-84fe-01193b11b573.png)
![dmsetatauvis](https://cloud.githubusercontent.com/assets/5736159/12592720/3dcdd568-c433-11e5-8587-1f24a02ed1db.png)
![dmeffvspt](https://cloud.githubusercontent.com/assets/5736159/12592719/3dca8f7a-c433-11e5-88c7-4bf1fbbadb7a.png)
comment
created
isobelojalvo
username_0
@username_2 Thanks for the plots. Could you tell me if red is from the current pull request or from the old? (or how can I check this myself?) Thank you!
comment
created
cvuosalo
username_2
@username_0: Yes, red in the plots is from the PR, and black is from the baseline I used, 800pre5.
comment
created
cvuosalo
username_2
@username_0: I checked AOD event content also, by using workflow 1000.0_RunMinBias2011A. Again, this PR adds 64 new tau discriminators, while only removing 16. We should be aiming to remove almost as many old, unneeded discriminators as new ones being added.
comment
created
slava77
username_1
comment
created
cvuosalo
username_2
@username_1: With this PR, there are 140 tau discriminators in AOD for workflow 1000.0_RunMinBias2011A.
comment
created
cvuosalo
username_2
@username_0: Concerning the unit test failures, the baseline used by Jenkins, CMSSW_8_0_X_2016-01-25-1100, performs the same unit tests successfully. It would seem this PR somehow adds the dependence on MVA6, as indicated by this error message:
```
Principal::getByToken: Found zero products matching all criteria
Looking for type: reco::PFTauDiscriminator
Looking for module label: hpsPFTauDiscriminationByMVA6LooseElectronRejection
```
comment
created
isobelojalvo
username_0
Hi @username_2, First: apologies about to go offline in a few minutes and will address other issues in the morning. Second: concerning the MVA6, yes, I see it is missing. However, I think this is due to the pattuplization test running on a 76x AOD sample. Dumping the event content of that file shows MVA6 is not there: This is the correct and expected behavior.
comment
created
slava77
username_1
comment
created
isobelojalvo
username_0
@username_1 @username_2 
Relval sample:
OK, we will see about the new relval sample. I am actually not used to the workflow for the creation of a new relval. Any guidance would help. I will also check with our tau relval experts. In the mean time, is it allowed to run on a privately produce sample instead? And can this pull request be integrated before the new relval is produced? (I am worrying about the timescale here)

Number of Discriminants:
We have a preference to do the discriminant clean up in a separate branch so that we can keep track if there is an error in the clean up. I created a new branch[*] in cms-tau-pog and will do a pull request when this branch is edited. It should be fast. 

DQM Plots:
It is expected that the efficiency will increase and this should be in line with what we see in 76xReminiAOD. In the plots, the low efficiency in the black was apparently due to the 3prong+1pi0 integration. It is removed in the current pull request. We are working towards reintegration in the NewDecayModes and not in the OldDecayModes.

[*]https://github.com/cms-tau-pog/cmssw/tree/80xReMiniAOD-RemoveOldDisc
comment
created
slava77
username_1
comment
created
slava77
username_1
About the unit tests, I was thinking that we could generate an AOD file on the fly for the case where we know well that the new AOD format is needed.

@gpetruc I seem to remember we did something to make the tests running PAT on AOD inputs to complete in a similar case. Do you remember what was done?
comment
created
isobelojalvo
username_0
The discriminants for removal are listed in the attached files created by Aruna

A rough count shows approximately 1/3 can certainly be removed. The increase overall is partially due to the addition of a new MVA6 electron discriminator and keeping the MVA5, we can remove MVA5 in the next cycle after analysts have had a chance to fully validate MVA6.

[PatTauID_Discriminators_80X.txt](https://github.com/cms-sw/cmssw/files/106993/PatTauID_Discriminators_80X.txt)
[RecoTauID_Discriminators_80X.txt](https://github.com/cms-sw/cmssw/files/106992/RecoTauID_Discriminators_80X.txt)
comment
created
cvuosalo
username_2
@username_0, @username_1: I am editing the list of tau discriminators here: https://twiki.cern.ch/twiki/bin/view/CMS/RecoTauDiscrim
comment
created
isobelojalvo
username_0
As suggested, I removed the new inputs. I will put them back in for pre6. Could this be tested?

Thanks
comment
created
slava77
username_1
@cmsbuild please test
comment
created
isobelojalvo
username_0
@username_2 concerning the twiki update, I have pointed Arun to the twiki and he will modify as necessary he has also already committed the cleaned up branch. Depending on the release schedule, it would be nice to have the cleaned up branch added to the next release. Let me know if this is a problem for you.
comment
created
cvuosalo
username_2
+1

For #13060 a20e68d1d9f843be26c134fcb066a63c992ff55f

Adds some new tau discriminators and removes a few obsolete ones. Also, removes the 3prong1pi0 decay mode from the AOD sequence.

Follow-up PRs will delete additional unneeded tau discriminators and will add new input for PAT algos after they become available in the next pre-release, as discussed above.

The code changes are satisfactory. Jenkins [tests](https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-13060/10818/summary.html) against baseline CMSSW_8_0_X_2016-01-27-2300 show numerous, insignificant differences related to tau decay modes, plus efficiency increases that are expected, as discussed above. Tests of workflow 25202.0_TTbar_13 with 70 events against baseline CMSSW_8_0_0_pre5 show similar differences as those from the Jenkins test results and the plots shown above. An additional example difference plot from this 25202 test is shown below.

Memory and timing measurements show no significant increase. Two timing measurements show no increase, while a third measurement shows an overall timing increase of 1%. At worst, the new tau discriminators may increase times by 18 ms per event, but we expect a follow-up PR that will delete more obsolete discriminators and reduce this timing increase.

Example of change in decay mode finding frequencies:
![dmfindsumpt](https://cloud.githubusercontent.com/assets/5736159/12689919/79e6a4fc-c6a5-11e5-8efe-cda581b6735e.png)
comment
created
cvuosalo
username_2
Additional timing measurements from test of workflow 25202 (without Validation step) against baseline CMSSW_8_0_0_pre5:

```
Total time formerly used by deleted discriminators: 2.8 ms/ev
Total time used by new discriminators: 62.3 ms/ev

New discriminators taking > 3 ms/ev
        hpsPFTauDiscriminationByLooseCombinedIsolationDBSumPtCorr3HitsdR03       4.90777 ms/ev
        hpsPFTauDiscriminationByMediumCombinedIsolationDBSumPtCorr3HitsdR03      4.88347 ms/ev
        hpsPFTauDiscriminationByTightCombinedIsolationDBSumPtCorr3HitsdR03       4.88874 ms/ev
        hpsPFTauDiscriminationByIsolationMVArun2v1PWnewDMwLTraw                  3.37712 ms/ev
        hpsPFTauPUcorrPtSumdR03                                                  4.82939 ms/ev
        hpsPFTauNeutralIsoPtSumWeightdR03                                        3.78463 ms/ev
        hpsPFTauDiscriminationByIsolationMVArun2v1DBdR03oldDMwLTraw              3.28005 ms/ev
        hpsPFTauDiscriminationByIsolationMVArun2v1PWdR03oldDMwLTraw              3.40165 ms/ev


Retained discriminator that uses the most time, change from baseline to PR:
        hpsPFTauDiscriminationByMVA5rawElectronRejection         15.8576 ms/ev -> 12.6722 ms/ev
```
comment
created
deguio
username_3
+1
comment
created
isobelojalvo
username_0
Do we (tau group) have any action items for this pull request? Can it be integrated?
comment
created
slava77
username_1
@username_0 @username_4 
analysis signature is still pending here.
There is one more PR (13100) that depends on integration of this one.
comment
created
davidlange6
username_4
Right - its been signed by other groups for only a few hours….

>
comment
created
isobelojalvo
username_0
No worries, just want to keep in mind the release closure cycle and not get 'ding-ed'. - Thanks
comment