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 to 94X: MET EE noise mitigation in 2017 data #25035

Merged
merged 23 commits into from
Nov 14, 2018

Conversation

eioannou
Copy link
Contributor

Backport of #24876

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 29, 2018

A new Pull Request was created by @eioannou (Emilios Ioannou) for CMSSW_9_4_X.

It involves the following packages:

JetMETCorrections/Type1MET
PhysicsTools/PatUtils

@perrotta, @monttj, @cmsbuild, @fgolf, @slava77, @peruzzim can you please review it and eventually sign? Thanks.
@TaiSakuma, @gouskos, @rappoccio, @HeinerTholen, @seemasharmafnal, @mmarionncern, @imarches, @ahinzmann, @smoortga, @acaudron, @jdolen, @drkovalskyi, @ferencek, @jdamgov, @nhanvtran, @gkasieczka, @schoef, @clelange, @JyothsnaKomaragiri, @mverzett, @gpetruc, @mariadalfonso, @pvmulder this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@perrotta
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 29, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/31330/console Started: 2018/10/29 14:54

self.setParameter('reclusterJets',True)

#ZD: puppi jet reclustering breaks the puppi jets
#overwriting of jet reclustering parameter for puppi
if self._parameters["Puppi"].value and not onMiniAOD:
self.setParameter('reclusterJets',False)

#jet collection overloading for automatic jet reclustering or JEC application
Copy link
Contributor

Choose a reason for hiding this comment

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

This was not removed in #24876, or later in the master: please check and fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @perrotta I didn't understand exactly what is this problem. Could you please help me on this?. I didn't remove this line in 94X.

Copy link
Contributor

Choose a reason for hiding this comment

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

These lines are not there any more: therefore, someone did remove them.

It was done by this commit from @kpedro88 :
4c2621a#diff-496fcf98e9a0901abbe07acea8ebda40

The same commit on the master only removed one empty line:
cms-met@6503137#diff-496fcf98e9a0901abbe07acea8ebda40

Please find out together with Kevin what happened there, and provide a fix either in 94X or in the master (or both...)

Copy link
Contributor

Choose a reason for hiding this comment

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

These lines were removed deliberately, because this default value was not actually used (overridden later), and its presence was confusing. It should have been removed in the upstream PR also. Perhaps there was a mistake in the rebase.

Copy link
Contributor

Choose a reason for hiding this comment

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

This makes sense. Please provide a PR in the master with the same removal then, so that the backport will remain synchronized with it.

@cmsbuild
Copy link
Contributor

@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-25035/31330/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 27
  • DQMHistoTests: Total histograms compared: 2721493
  • DQMHistoTests: Total failures: 108
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2721223
  • DQMHistoTests: Total skipped: 162
  • DQMHistoTests: Total Missing objects: 0

@perrotta
Copy link
Contributor

perrotta commented Nov 5, 2018

@eioannou please inform here as soon as the PR with the missing piece in the master is finally submitted, so that we can proceed with this backport

@eioannou
Copy link
Contributor Author

eioannou commented Nov 5, 2018

Hi @perrotta should I make a new PR in the master removing those lines?

@perrotta
Copy link
Contributor

perrotta commented Nov 5, 2018 via email

@eioannou
Copy link
Contributor Author

eioannou commented Nov 5, 2018

Hi @perrotta, I have created a new PR #25129 for the master by removing the unnecessary lines in order to be consistent with this backport.

@perrotta
Copy link
Contributor

perrotta commented Nov 9, 2018

+1

  • Faithful backport of MET EE noise mitigation in 2017 data #24876 and MET EE noise mitigation in 2017 data [remove unnecessary lines] #25129
  • A new filter is made available for users.
    • If the parameters in PhysicsTools/PatUtils/python/tools/runMETCorrectionsAndUncertainties.py are kept at their default value the newly added filter is disabled, and therefore no effect can be propagated to the standard workflows (and no effect is seen in the results of the jenkins tests, in fact)
    • As such the pull request can be merged without modifying the default behaviour, and it is therefore suitable for being backported

@peruzzim
Copy link
Contributor

peruzzim commented Nov 9, 2018

We need this ported to 10_2_X as well asap, @eioannou could you please open the PR already?

@eioannou
Copy link
Contributor Author

eioannou commented Nov 9, 2018

@peruzzim the PR for 10_2_X is #25173

@fabiocos
Copy link
Contributor

+1

@fabiocos
Copy link
Contributor

merge

no change to default expected

@cmsbuild cmsbuild merged commit 35c6665 into cms-sw:CMSSW_9_4_X Nov 14, 2018
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.

6 participants