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

Updates for TauReco at miniAOD #30571

Merged

Conversation

mbluj
Copy link
Contributor

@mbluj mbluj commented Jul 7, 2020

PR description:

This PR contains updates for Tau reconstruction on top of MiniAOD.
The main change is addition to the miniAOD event content energy fractions of PFGammas (packedCandidates with pdgId==22). This completes information needed to build tau quantities based on energy fractions of its constituents. Other changes introduced by the PR are use of the energy fractions:

  • updates of tauID against-mu simple: usage of previously added energy fractions for charged hadrons and code reorganisation to use new tauID dataformat + fillDescription;
  • anti-e MVA6 and deepTauID (both using energy fractions of charged hadrons and gammas) added to the tauReco-at-MiniAOD workflow;
  • recently moderinised tauID against-e-in-deadECal added back to the tauReco-at-MiniAOD workflow;

PR validation:

Matrix tests (runTheMatrix.py -l limited -i all --ibeos) successful.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 7, 2020

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 7, 2020

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30571/16808

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 7, 2020

A new Pull Request was created by @mbluj for master.

It involves the following packages:

PhysicsTools/PatAlgos
RecoTauTag/Configuration
RecoTauTag/RecoTau

@perrotta, @jpata, @cmsbuild, @santocch, @slava77 can you please review it and eventually sign? Thanks.
@rappoccio, @gouskos, @hatakeyamak, @emilbols, @peruzzim, @seemasharmafnal, @mmarionncern, @ahinzmann, @smoortga, @jdolen, @ferencek, @jdamgov, @nhanvtran, @gkasieczka, @schoef, @andrzejnovak, @clelange, @riga, @JyothsnaKomaragiri, @mverzett, @gpetruc, @mariadalfonso this is something you requested to watch as well.
@silviodonato, @dpiparo you are the release manager for this.

cms-bot commands are listed here

@slava77
Copy link
Contributor

slava77 commented Jul 7, 2020

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 7, 2020

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 7, 2020

+1
Tested at: 341f521
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c46113/7749/summary.html
CMSSW: CMSSW_11_2_X_2020-07-06-2300
SCRAM_ARCH: slc7_amd64_gcc820

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 7, 2020

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 7, 2020

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c46113/7749/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 77 differences found in the comparisons
  • DQMHistoTests: Total files compared: 37
  • DQMHistoTests: Total histograms compared: 2787364
  • DQMHistoTests: Total failures: 14
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 2787299
  • DQMHistoTests: Total skipped: 50
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.004 KiB( 36 files compared)
  • DQMHistoSizes: changed ( 10224.0 ): 0.004 KiB MessageLogger/Warnings
  • Checked 154 log files, 17 edm output root files, 37 DQM output files

@santocch
Copy link

+1

@slava77
Copy link
Contributor

slava77 commented Jul 15, 2020

The main change is addition to the miniAOD event content energy fractions of PFGammas (packedCandidates with pdgId==22).

please provide event size cost information of this update

@slava77
Copy link
Contributor

slava77 commented Jul 15, 2020

assign xpog
to sign off on the miniAOD output size increase

The main change is addition to the miniAOD event content energy fractions of PFGammas (packedCandidates with pdgId==22).

please provide event size cost information of this update

in 100 events of wf 136.88811 available in jenkins outputs
the packedCandidates size is up by 83 bytes/event or 0.1% of the file size.

1K events or more would be a safer check though, although I doubt that it would change the fraction to above 0.2%

@cmsbuild
Copy link
Contributor

New categories assigned: xpog

@fgolf,@mariadalfonso,@gouskos,@peruzzim you have been requested to review this Pull request/Issue and eventually sign? Thanks

@slava77
Copy link
Contributor

slava77 commented Aug 12, 2020

BTW, could indentation issues of this kind ("TabError: inconsistent use of tabs and spaces in indentation") be automatically detected with code-format or code-style (or similar) checks?

I opened #30775 a few weeks ago but there was no traction yet

@cmsbuild
Copy link
Contributor

+1
Tested at: 7b7ad32
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c46113/8728/summary.html
CMSSW: CMSSW_11_2_X_2020-08-12-1100
SCRAM_ARCH: slc7_amd64_gcc820

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c46113/8728/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 25 differences found in the comparisons
  • DQMHistoTests: Total files compared: 35
  • DQMHistoTests: Total histograms compared: 2612401
  • DQMHistoTests: Total failures: 5
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 2612347
  • DQMHistoTests: Total skipped: 48
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.004 KiB( 34 files compared)
  • DQMHistoSizes: changed ( 10224.0 ): 0.004 KiB MessageLogger/Warnings
  • Checked 149 log files, 22 edm output root files, 35 DQM output files

@slava77
Copy link
Contributor

slava77 commented Aug 14, 2020

DeepTau does not use energy fractions neither of pf-gammas nor pf-charged (the latter were also introduced after deepTau has been trained); the only used fractions are for isolated charged hadrons and neutral hadrons. Therefore, I suppose that the difference is caused by some indirect dependence - I am checking it.

@mbluj
please clarify if the check was done and did not show any issues to be fixed in this PR
Thank you.

@slava77
Copy link
Contributor

slava77 commented Aug 14, 2020

DeepTau does not use energy fractions neither of pf-gammas nor pf-charged (the latter were also introduced after deepTau has been trained); the only used fractions are for isolated charged hadrons and neutral hadrons. Therefore, I suppose that the difference is caused by some indirect dependence - I am checking it.

@mbluj
please clarify if the check was done and did not show any issues to be fixed in this PR
Thank you.

nevermind, this PR does not modify deepTau values anymore

@slava77
Copy link
Contributor

slava77 commented Aug 14, 2020

+1

for #30571 7b7ad32

  • code changes are in line with the PR description and the follow up review
    • general miniAOD is updated to have hcalFraction and caloFraction saved for photons (pt>0.5 GeV); this is estimated to increase the size of packedPFCandidates by 0.3% Updates for TauReco at miniAOD #30571 (comment)
    • other changes are affecting tau reco from miniAOD, which is not in the cms-bot tests
  • jenkins tests pass and comparisons with the baseline show differences only in the packedPFCandidates caloFraction (curiously, in with the statistics of the tests there were no changes in the hcalFraction variable)

@mbluj
Copy link
Contributor Author

mbluj commented Aug 17, 2020

DeepTau does not use energy fractions neither of pf-gammas nor pf-charged (the latter were also introduced after deepTau has been trained); the only used fractions are for isolated charged hadrons and neutral hadrons. Therefore, I suppose that the difference is caused by some indirect dependence - I am checking it.

@mbluj
please clarify if the check was done and did not show any issues to be fixed in this PR
Thank you.

nevermind, this PR does not modify deepTau values anymore

Yes indeed an issue affecting deepTauID (not diectly related with the energy fractions) has been introduced in this PR, and then fixed.

@santocch
Copy link

+1

@slava77
Copy link
Contributor

slava77 commented Aug 24, 2020

@cms-sw/xpog-l2
please check this PR, your signature is needed.
Thank you.

@mariadalfonso
Copy link
Contributor

+xpog
0.3% increase for packedPFCandidates is acceptable

Size increase for MiniAOD on top of 9k ttbar relval Run-3 events [1] with the updated setup compared to plain CMSSW_11_2_0_pre1; configuration with cmsDriver command specified in [2].

  • total size: 702258766/701292254 bytes = 1.0014, i.e. +0.14% or +107 bytes/event,
  • packedPFCandidates: +0.3% (output from compareProducts.sh [3]).

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@qliphy
Copy link
Contributor

qliphy commented Aug 25, 2020

+1

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

7 participants