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 for Puppi MET and MET significance compatibility between AOD and miniAOD (80X) #16174

Closed
wants to merge 58 commits into from

Conversation

@mmarionncern
Copy link

mmarionncern commented Oct 11, 2016

From what I understood, there is some space for pushing small PRs in 8_0_21 that will be used for MC production as L1 is still missing.

That PR is similar to #16140 for 80X. If we have the opportunity to push it in the release rather than offering a private recipe as it was planned by the MET group, that would be probably less confusing for the analysers.
(N.B. this PR touches the reco sequence, let me know if we cannot push this PR due to that)

@cmsbuild cmsbuild added this to the Next CMSSW_8_0_X milestone Oct 11, 2016
@cmsbuild

This comment has been minimized.

Copy link
Contributor

cmsbuild commented Apr 7, 2017

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 23 differences found in the comparisons
  • DQMHistoTests: Total files compared: 16
  • DQMHistoTests: Total histograms compared: 1164243
  • DQMHistoTests: Total failures: 1321
  • DQMHistoTests: Total nulls: 43
  • DQMHistoTests: Total successes: 1162761
  • DQMHistoTests: Total skipped: 118
  • DQMHistoTests: Total Missing objects: 0
  • Checked 63 log files, 14 edm output root files, 16 DQM output files
cmsbuild added a commit that referenced this pull request Apr 11, 2017
Port of the last 80X/90X MET (#16174 #17524) changes in 91X
davidlange6 added a commit that referenced this pull request Apr 11, 2017
New MET filters (part 1 of #16174 with #17720 developments) for 80X
@slava77

This comment has been minimized.

Copy link
Contributor

slava77 commented Apr 11, 2017

@mmarionncern
now that #17968 is merged , please remove changes from this PR that are already in 80X.
This will be needed towards miniAODv2.
Thank you.

@mmarionncern

This comment has been minimized.

Copy link
Author

mmarionncern commented Apr 12, 2017

@slava77, as the relevant commits are the same as the ones of #17968, I am not sure to understand why the changes should be removed from the PR. The merge should take care automatically of the commit "duplication". Can you clarify?

Thanks

@slava77

This comment has been minimized.

Copy link
Contributor

slava77 commented Apr 12, 2017

@cmsbuild

This comment has been minimized.

Copy link
Contributor

cmsbuild commented Apr 12, 2017

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

@mmarionncern

This comment has been minimized.

Copy link
Author

mmarionncern commented Apr 12, 2017

Done, the remnant merge commit is not reverted to avoid touching files that were not touched in #17968

@slava77

This comment has been minimized.

Copy link
Contributor

slava77 commented Apr 12, 2017

diffs show files like 141 RecoMET/METFilters/plugins/BadChargedCandidateSummer16Filter.cc
IIUC, these do not belong in here

@mmarionncern

This comment has been minimized.

Copy link
Author

mmarionncern commented Apr 12, 2017

@slava77 That file is not overlapping with #17968, this is part of the first commits of the PR.

@slava77

This comment has been minimized.

Copy link
Contributor

slava77 commented Apr 12, 2017

@mmarionncern

This comment has been minimized.

Copy link
Author

mmarionncern commented Apr 12, 2017

Either I remove them and do an extra merge with the current 80X branch, which basically make this PR similar to its previous status, or I remove the filter calls from the config files as otherwise the current branch won't be able to run in a standalone way as it needs at least one version of the filter.

@slava77

This comment has been minimized.

Copy link
Contributor

slava77 commented Apr 12, 2017

@mmarionncern

This comment has been minimized.

Copy link
Author

mmarionncern commented Apr 12, 2017

Either the BadChargedCandidateFilter or the old filters that are still in this PR are needed to run the filters defined in
https://github.com/cms-sw/cmssw/blob/CMSSW_8_0_X/RecoMET/METFilters/python/metFilters_cff.py

In this PR, the former module does not exists and the PR includes a call to the previous version of the filter, in the same way as the latest an greatest one :
https://github.com/cms-sw/cmssw/pull/16174/files#diff-09b1dcadbdc74bad980084e7fc64628d

If I remove the old filters, I have to remove the call from the configuration as well, and thus propagate that change to 80X, which is not something wanted. If I do not remove the call, the PR is not self-consistent as filters are missing.

To be correct, I would need to merge the 80X content that is not yet in this PR, but that would lead to the same situation as the one it was this morning, with the inclusion of #17968 stuff in that PR.

@slava77

This comment has been minimized.

Copy link
Contributor

slava77 commented Apr 12, 2017

@mmarionncern

This comment has been minimized.

Copy link
Author

mmarionncern commented Apr 12, 2017

I am not saying that the BadChargedCandidateFilter is called, I am saying that the removal of the filters in that PR would necessitate the removal of the following config lines
https://github.com/cms-met/cmssw/blob/eec42610ed5039958968488755acf27723ab4e69/RecoMET/METFilters/python/metFilters_cff.py#L97-L100

which is not wanted in CMSSW, as we explicitely added them in 80X with the #17968 PR already.

I would be happy to remove the old filters that are indeed not needed anymore, but those lines cannot be removed

@mmarionncern

This comment has been minimized.

Copy link
Author

mmarionncern commented Apr 12, 2017

Other possibility, rebase that branch on top of the last 80X developments and recreate a new one

@slava77

This comment has been minimized.

Copy link
Contributor

slava77 commented Apr 12, 2017

@mmarionncern

This comment has been minimized.

Copy link
Author

mmarionncern commented Apr 12, 2017

Rebased on top of 80X in #18341
Closing this one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.