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

Standard muon selectors #20612

Merged
merged 32 commits into from Oct 8, 2017
Merged

Conversation

drkovalskyi
Copy link
Contributor

@drkovalskyi drkovalskyi commented Sep 21, 2017

Standard Muon Selectors - critical addition for 94X from Muon POG
Passed default tests and no problems were found running matrix.
Latest presentation on the subject in the general muon POG meeting:
https://indico.cern.ch/event/666069/contributions/2721346/attachments/1524991/2384210/muon_pog_standard_selectors_for_MC2017v2.pdf

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-20612/869

Code check has found code style and quality issues which could be resolved by applying a patch in https://cmssdt.cern.ch/SDT/code-checks/PR-20612/869/git-diff.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/PR-20612/869/git-diff.patch | patch -p1

You can run scram build code-checks to apply code checks directly (this will soon be required for PRs to be considered)

@slava77
Copy link
Contributor

slava77 commented Sep 21, 2017 via email

@slava77
Copy link
Contributor

slava77 commented Sep 21, 2017

@drkovalskyi
please fix issues in the code-checks.
This is now a requirement for PRs and we can not run automated tests of the code without them.

@@ -183,6 +183,43 @@ namespace reco {
RPCHitAndTrackArbitration, GEMSegmentAndTrackArbitration, ME0SegmentAndTrackArbitration };

///
/// ====================== STANDARD SELECTORS ===========================
///
enum Selector {
Copy link
Contributor

Choose a reason for hiding this comment

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

IDSelector may be more descriptive

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not ID. It's any selector including isolation.

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 6, 2017

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 6, 2017

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-20612/1206

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 6, 2017

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

@perrotta
Copy link
Contributor

perrotta commented Oct 6, 2017

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 6, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/23511/console Started: 2017/10/06 19:30

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 6, 2017

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 6, 2017

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 26
  • DQMHistoTests: Total histograms compared: 2761427
  • DQMHistoTests: Total failures: 291
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2760949
  • DQMHistoTests: Total skipped: 187
  • DQMHistoTests: Total Missing objects: 0
  • Checked 107 log files, 10 edm output root files, 26 DQM output files

@slava77
Copy link
Contributor

slava77 commented Oct 6, 2017

It looks like the conflicts could have been avoided by a cleaner removal of the timing changes made and then taken out a week ago (as suggested in #20612 (comment))

Even before the merge conflicts appeared the commit graph in this PR already had about 3 cycles with 2 merges points with the CMSSW upstream.
IMHO, keeping a linear history is a much more straightforward approach.

@slava77
Copy link
Contributor

slava77 commented Oct 6, 2017

+1

for #20612 0dce48d

  • update to resolve somewhat non-existent conflicts with the edits in CSCTimingExtractor.cc, DTTimingExtractor.cc, MuonTimingFiller.cc merged upstream recently. This is technical with respect to the previous signoff point.
  • jenkins tests pass

@davidlange6
Copy link
Contributor

merge

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

8 participants