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

Fix bug affecting boosted tau isolation (10_6_X) #30066

Merged
merged 3 commits into from Jun 4, 2020

Conversation

mbluj
Copy link
Contributor

@mbluj mbluj commented Jun 1, 2020

PR description:

This PR fixes a bug affecting indexing jet-constituent-maps. In the result of the bug constituents of a subjet nearby a considered tau - a seed of a nearby tau candidate - are not excluded from particles used to compute isolation of the tau, which breaks the boosted tau algorithm. The bug has been introduced in #17087 (for 90X) speeding up boosted tau seed producer code. Unfortunately the bug was discovered only recently by users (*).
In addition to the bug fix, in this PR we remove remnants of previous implementation changed by #17087 and improve code formatting.

Expected changes: boosted taus (in MiniAOD), especially those with other tau candidate within R<0.5, are more isolated. This will recover state from before #17087 as shown in the figure attached to #30064.

Backport of #30064 to 10_6_X. Some additional details therein.

PR validation:

Tested with the standard miniAOD workflow.

if this PR is a backport please specify the original PR and why you need to backport that PR:

Backport of #30064

…7087; remove remnants of previous implementation; code formatting
@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 1, 2020

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

It involves the following packages:

RecoJets/JetProducers

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@rappoccio, @jdolen, @ahinzmann, @jdamgov, @yslai, @nhanvtran, @gkasieczka, @clelange, @schoef, @mariadalfonso, @seemasharmafnal 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 Jun 1, 2020

@mbluj
please update the PR title ("ix" -> "fix").

Do I understand correctly that this update is violating the no-change policy?
(or is the modified code enabled only on analysis jobs running on miniAOD)
If the regular miniAOD production is affected, please update to use run_miniAOD_devel

@mbluj mbluj changed the title ix bug affecting boosted tau isolation (10_6_X) Fix bug affecting boosted tau isolation (10_6_X) Jun 1, 2020
@mbluj
Copy link
Contributor Author

mbluj commented Jun 1, 2020

@mbluj
please update the PR title ("ix" -> "fix").

Do I understand correctly that this update is violating the no-change policy?
(or is the modified code enabled only on analysis jobs running on miniAOD)
If the regular miniAOD production is affected, please update to use run_miniAOD_devel

Hmm, yes, it changes isolationCandidates of boosted taus and their isolation-related tauIDs, but not taus itself. In this sense this breaks compatibility with already produced 106X miniAOD samples. It is a bit strange to introduce a configuration parameter with switches on/off a bug, but of course I will introduce it.

@perrotta
Copy link
Contributor

perrotta commented Jun 1, 2020

backport of #30064

@mbluj
Copy link
Contributor Author

mbluj commented Jun 1, 2020

Adding @abdollah110 who found the bug.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 1, 2020

Pull request #30066 was updated. @perrotta, @cmsbuild, @santocch, @slava77 can you please check and sign again.

@mbluj
Copy link
Contributor Author

mbluj commented Jun 1, 2020

@Slava: a flag enabling/disabling fixed behaviour of the boosted tau seeding is added in the most recent commit. Please consider to add this fix to the list of UL reMiniAOD features.

@slava77 slava77 mentioned this pull request Jun 1, 2020
41 tasks
@silviodonato
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 2, 2020

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/6739/console Started: 2020/06/02 16:07

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 2, 2020

+1
Tested at: 0b51527
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-188443/6739/summary.html
CMSSW: CMSSW_10_6_X_2020-06-02-1100
SCRAM_ARCH: slc7_amd64_gcc700

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 2, 2020

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 2, 2020

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 33
  • DQMHistoTests: Total histograms compared: 3212324
  • DQMHistoTests: Total failures: 19
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3211971
  • DQMHistoTests: Total skipped: 334
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 32 files compared)
  • Checked 137 log files, 14 edm output root files, 33 DQM output files

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 3, 2020

Pull request #30066 was updated. @perrotta, @cmsbuild, @slava77 can you please check and sign again.

@perrotta
Copy link
Contributor

perrotta commented Jun 3, 2020

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 3, 2020

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/6769/console Started: 2020/06/03 14:55

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 3, 2020

+1
Tested at: c018fb4
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-188443/6769/summary.html
CMSSW: CMSSW_10_6_X_2020-06-03-1100
SCRAM_ARCH: slc7_amd64_gcc700

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 3, 2020

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 3, 2020

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 33
  • DQMHistoTests: Total histograms compared: 3212324
  • DQMHistoTests: Total failures: 19
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3211971
  • DQMHistoTests: Total skipped: 334
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 32 files compared)
  • Checked 137 log files, 14 edm output root files, 33 DQM output files

@perrotta
Copy link
Contributor

perrotta commented Jun 3, 2020

+1

  • The fix acts as it should, following the PR description and the intention behind the original code
  • The update differs from the one submitted in the master because:
  • Jenkins tests pass and show no differences for the standard workflows, as it should

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 3, 2020

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

@silviodonato
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 6b190bd into cms-sw:CMSSW_10_6_X Jun 4, 2020
@mbluj mbluj deleted the CMSSW_10_6_X-boostedTauSeedFix branch March 13, 2024 13:28
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

5 participants