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

Package of several technical changes: downgrade of edm::Warnings to edm::Info, removal of an unused parameter from cfi file, upward move of jet selection for tau seeding, switch off of production of null taus #17620

Merged

Conversation

roger-wolf
Copy link
Contributor

@roger-wolf roger-wolf commented Feb 23, 2017

Hi,

I am citing Michal's comment to this pull request below (*). Matrix tests have been performed and were positive. This is pure technical work that should not touch the physics performance. This is also what our tests indicated. Runtime performance might change e.g. since edm::Warnings are dowgrade to edm::Infos.

Cheers,
Roger

(*)
The following issues are fixed with this PR:

  • Warning messages about null jet area are downgraded in the PFRecoTauDiscriminationAgainstMuon2. In addition settings of the discriminant for boosted tau reconstruction are corrected.
  • A selection is added for jets seeding production of JetRegions, TauChargedHadron and PiZero candidates identical to the one used by TauProducer. This avoids production of JetRegions, TauChargedHadrons and PiZeros which are then unused by Tau producer module which is more economical (less products stored in memory, less loops over PFCandidates, etc.)
  • Unused configuration parameter is removed from PFRecoTauChargedHadronProducer_cfi.py
  • Production of null taus, i.e. jets which are not all compatible with tau, is switched off in RecoTauTag/RecoTau/python/RecoTauCombinatoricProducer_cfi.py

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @roger-wolf (Roger Wolf) for CMSSW_9_0_X.

It involves the following packages:

CommonTools/ParticleFlow
RecoTauTag/Configuration
RecoTauTag/RecoTau

@cmsbuild, @cvuosalo, @slava77, @monttj, @davidlange6 can you please review it and eventually sign? Thanks.
@ahinzmann, @jdolen, @rappoccio, @gkasieczka, @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 Feb 23, 2017

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 23, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/17941/console Started: 2017/02/23 18:15

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Feb 25, 2017

@roger-wolf
please modify the title to describe what is changed rather than to say that it has no effect

@roger-wolf roger-wolf changed the title Pure technical fixes that are supposed to have no influence on physics performance Package of several technical changes: downgrade of edm::Warnings to edm::Info, removal of an unused parameter from cfi file, upward move of jet selection for tau seeding, switch off of production of null taus Feb 27, 2017
@roger-wolf
Copy link
Contributor Author

Hi Slava,

I hope this satisfies your justified formal request. Comments/confirmations on the more relevant physics question that you are having I've to leave to Michal.

Cheers,
Roger

@mbluj
Copy link
Contributor

mbluj commented Feb 27, 2017

Hi Slava,

probably what causes confusion is the name "null taus"; Null taus are not taus with zero momentum, but taus build from jets where nothing tau-like was found. Such a tau carries 4-momentum of original seeding jet (pt>14 & |eta|<2.4 by construction) while other data members of PFTau object are not filled thus tau is null (for instance does not possess signal candidates). Null taus were produced for debugging (in this way there is 1-to-1 correspondence between seeding jets and PFtaus) and should be switched off long time ago. Anyway, such taus are not used by analysts and not stored in MiniAOD.

There is also an another class of taus which are not quite tau-like. They have an reconstructed decay mode, but it is very poor, e.g. a signal charged hadron candidate for unique tau "prong" is substituted by neutral candidate matched to a (poor) general track. Such taus have usually low or even undefined (null) momentum. Those should be also eliminated, but it requires an additional selection (at least one signal charged hadron to be required). And again, such taus are not stored in MiniAOD.

So, propose to revert buildNullTaus to True in this PR and prepare a new PR with set it to false and an additional selection to kill the other type of bad taus. Alternatively, the additional section can be added here, if it is your preference.

@slava77
Copy link
Contributor

slava77 commented Feb 27, 2017 via email

@slava77
Copy link
Contributor

slava77 commented Feb 27, 2017 via email

@cmsbuild
Copy link
Contributor

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

@mbluj
Copy link
Contributor

mbluj commented Feb 27, 2017

buildNullTaus reverted to previous setting (True) in RecoTauTag/RecoTau/python/RecoTauCombinatoricProducer_cfi.py
Updated configuration will be a subject of other PR

@slava77
Copy link
Contributor

slava77 commented Feb 27, 2017

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 27, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/17987/console Started: 2017/02/27 15:17
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/17988/console Started: 2017/02/27 15:22

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Feb 27, 2017

+1

for #17620 8dd7bd3

  • technical updates in line with the description
  • jenkins tests pass
    • comparisons with baseline show no differences
    • logs of 136.731 step3 show that the warning with has area = 0 are gone

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.

5 participants