ME0MuonSelector cleanup pass #31190

davidlange6 · 2020-08-20T07:15:28Z

The modules IB pointed out duplicated structs between MuonSelector and ME0MuonSelector. To avoid this, I added a namespace to the me0 set of functions for lack of better idea.

There were in addition #includes of plugin/*.cc files which have been cleaned up and rearranged to avoid duplicate plugin problems. There may be other issues not fixed by this PR.. [Perhaps #including cc files can be prevented by code checks...:)

cmsbuild · 2020-08-20T07:15:53Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-08-20T07:22:05Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31190/17828

This PR adds an extra 40KB to repository

cmsbuild · 2020-08-20T07:22:30Z

A new Pull Request was created by @davidlange6 (David Lange) for master.

It involves the following packages:

CommonTools/RecoAlgos
RecoMuon/MuonIdentification

@perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@rappoccio, @clelange, @jdolen, @makortel, @ahinzmann, @folguera, @riga, @abbiendi, @jhgoh, @echapon, @calderona, @HuguesBrun, @gkasieczka, @Fedespring, @hatakeyamak, @bellan, @trocino, @cericeci, @rociovilar this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

davidlange6 · 2020-08-20T08:07:03Z

please test

cmsbuild · 2020-08-20T08:07:24Z

The tests are being triggered in jenkins.

guitargeek · 2020-08-20T08:48:25Z

Hi @davidlange6, I have to mention something else related to this ME0MuonSelector.

It is used in CommonTools/RecoAlgos/plugins/ME0MuonTrackCollProducer.cc, which is used by CommonTools/RecoAlgos/python/me0Associator.py. This python file is in turn not used in production, there is only a test file in RecoMuon/MuonIdentification/test/testME0MuonAnalyzer_Example.py. Note this is again in the MuonIdentification package, just like the ME0MuonSelector that you change in this PR.

Therefore, we have a rather awkward dependency relation between be packages: CommonTools uses RecoMuon to define a plugin+configuration that is not common at all, and then the tests of this are again in RecoMuon.

Would you also think it's better to have everything in RecoMuon to avoid the dependency of CommonTools on reco? I think, CommonTools should ideally be higher up in the dependency hierarchy.

In practice, this suggestion implies to move:
CommonTools/RecoAlgos/plugins/ME0MuonTrackCollProducer.cc -> RecoMuon/MuonIdentification/plugins/ME0MuonTrackCollProducer.cc
CommonTools/RecoAlgos/python/me0Associator.py -> RecoMuon/MuonIdentification/python/me0Associator.py

perrotta · 2020-08-20T09:11:24Z

By the way, something else that could be cleaned straightforwardly in this ME0MuonTrackCollProducer.cc is the following (just listing what jumps to the eyes as being obviously uselessly included):

Remove the duplicated

#include "DataFormats/TrackReco/interface/Track.h"
#include "DataFormats/TrackReco/interface/TrackFwd.h"

Remove the unneeded dependencies on

#include "DataFormats/MuonDetId/interface/CSCDetId.h"
#include "DataFormats/MuonDetId/interface/DTChamberId.h"
...
#include "DataFormats/CSCRecHit/interface/CSCSegmentCollection.h"
#include "DataFormats/DTRecHit/interface/DTRecSegment4DCollection.h"

Remove the unneeded dependency on:

#include "DataFormats/HepMCCandidate/interface/GenParticle.h"

Remove the unneeded dependencies on

#include "DataFormats/VertexReco/interface/Vertex.h"
#include "DataFormats/VertexReco/interface/VertexFwd.h"

as well as the commented out line

  //std::vector<double> findSimVtx(edm::Event& iEvent);

perrotta · 2020-08-20T13:49:01Z

@Fedespring @cericeci : profiting of this PR, could you plese check if all those "ME0" muons stuffs are still needed at all in the standard workflows (or any other "local" one)? If not, this could be a good opportunity to start cleaning all them out

davidlange6 · 2020-08-24T09:59:03Z

Would you also think it's better to have everything in RecoMuon to avoid the dependency of CommonTools on reco? I think, CommonTools should ideally be higher up in the dependency hierarchy.

Clearly that would be an improvement, yes. [but I don't really follow the design logic (or why we would need a ME0 specific muon selector)] Probably best addressed by those responsible for muon selectors?

davidlange6 · 2020-08-24T10:00:19Z

please test

cmsbuild · 2020-08-24T10:00:39Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-08-23-2300/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/8874/console Started: 2020/08/24 12:38

cmsbuild · 2020-08-24T10:46:23Z

-1

Tested at: b6acc69

CMSSW: CMSSW_11_2_X_2020-08-23-2300
SCRAM_ARCH: slc7_amd64_gcc820
You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6a2320/8874/summary.html

I found follow errors while testing this PR

Failed tests: Build

Build:

I found compilation error when building:

>> Compiling edm plugin /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_2_X_2020-08-23-2300/src/CommonTools/RecoAlgos/plugins/VertexRefSelector.cc
>> Compiling edm plugin /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_2_X_2020-08-23-2300/src/CommonTools/RecoAlgos/plugins/VertexSelector.cc
>> Building  edm plugin tmp/slc7_amd64_gcc820/src/CommonTools/RecoAlgos/plugins/CommonToolsRecoAlgos_plugins/libCommonToolsRecoAlgos_plugins.so
/cvmfs/cms-ib.cern.ch/nweek-02643/slc7_amd64_gcc820/external/gcc/8.2.0-bcolbf/bin/../lib/gcc/x86_64-unknown-linux-gnu/8.4.0/../../../../x86_64-unknown-linux-gnu/bin/ld: tmp/slc7_amd64_gcc820/src/CommonTools/RecoAlgos/plugins/CommonToolsRecoAlgos_plugins/ME0MuonTrackCollProducer.cc.o: in function `ME0MuonTrackCollProducer::produce(edm::Event&, edm::EventSetup const&)':
ME0MuonTrackCollProducer.cc:(.text+0x512): undefined reference to `muon::me0::isGoodMuon(reco::ME0Muon const&, muon::me0::SelectionType)'
collect2: error: ld returned 1 exit status
gmake: *** [tmp/slc7_amd64_gcc820/src/CommonTools/RecoAlgos/plugins/CommonToolsRecoAlgos_plugins/libCommonToolsRecoAlgos_plugins.so] Error 1
Leaving library rule at src/CommonTools/RecoAlgos/plugins
Entering library rule at CommonTools/RecoAlgos
>> Compiling  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_2_X_2020-08-23-2300/src/CommonTools/RecoAlgos/src/ClusterStorer.cc
>> Compiling  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_2_X_2020-08-23-2300/src/CommonTools/RecoAlgos/src/MassiveCandidateConverter.cc

cmsbuild · 2020-08-24T10:46:24Z

Comparison not run due to Build errors (RelVals and Igprof tests were also skipped)

davidlange6 · 2020-08-24T15:57:55Z

Odd, I had to reload to see the word “fatal” in that log.. ok, I will restart the tests.. On Aug 24, 2020, at 5:52 PM, Matti Kortelainen <notifications@github.com<mailto:notifications@github.com>> wrote: Hmm, I see at the end of https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6a2320/8878/addOnTests/hlt_data_GRun/cmsDriver.py_RelVal_-s_L1REPACK:Full_--data_--scenario=pp_-n_10_--conditions_auto:run3_hlt_GRun_--relval_9000,50_--datatier_RAW_--eventcontent_RAW_-.log only Fatal system signal has occurred during exit We've seen that symptom before on cc8 that lead to #30432<#30432>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#31190 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABGPFQ5VWP5R76B2TF4NFB3SCKEEPANCNFSM4QFXM3EA>.

davidlange6 · 2020-08-24T15:58:06Z

please test

cmsbuild · 2020-08-24T15:58:13Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-08-24-1100/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/8888/console Started: 2020/08/24 17:59

cmsbuild · 2020-08-24T17:15:38Z

+1
Tested at: 046da59
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6a2320/8888/summary.html
CMSSW: CMSSW_11_2_X_2020-08-24-1100
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-08-24T17:15:41Z

Comparison job queued.

cmsbuild · 2020-08-24T18:43:35Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 35
DQMHistoTests: Total histograms compared: 2609656
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2609633
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
Checked 149 log files, 22 edm output root files, 35 DQM output files

perrotta · 2020-09-02T16:15:42Z

I don't think this simple PR should be delayed any further,
@trocino and Arnab Purohit (please provide the github name of Arnab, to be listed among the reco contacts), who are replacing @Fedespring and @cericeci, please virify independently from this PR if there is any ME0 related code that can be removed from the release, so that there will be no need to maintain it further in the future

perrotta · 2020-09-02T16:18:00Z

+1

Technical
Jenkins tests pass
Further cleanings and rearrangements of the code were suggested in the thread: they can be implemented in a separate PR, or even not needed in case some useless code can be removed from the release (up to the muon POG reco contacts to verify it)

cmsbuild · 2020-09-02T16:18:25Z

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 · 2020-09-03T01:09:57Z

+1

davidlange6 added 2 commits August 20, 2020 09:11

cleanup strange and duplicated structures in ME0MuonSelector

0d9caf2

apply code checks

b6acc69

cmsbuild added this to the CMSSW_11_2_X milestone Aug 20, 2020

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Aug 20, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 20, 2020

cmsbuild added tests-started and removed tests-pending labels Aug 20, 2020

cmsbuild added tests-pending and removed tests-started labels Aug 24, 2020

cmsbuild added tests-started and removed tests-pending labels Aug 24, 2020

cmsbuild added comparison-pending tests-pending and removed comparison-available tests-rejected labels Aug 24, 2020

cmsbuild added tests-started and removed tests-pending labels Aug 24, 2020

cmsbuild added tests-approved and removed tests-started labels Aug 24, 2020

cmsbuild added comparison-available and removed comparison-pending labels Aug 24, 2020

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Sep 2, 2020

cmsbuild added orp-approved and removed orp-pending labels Sep 3, 2020

cmsbuild merged commit 88f3438 into cms-sw:master Sep 3, 2020

cmsbuild mentioned this pull request Sep 3, 2020

DQM: Remove concurrent-lumi blocking DQM Modules. #30111

Closed

perrotta mentioned this pull request Sep 8, 2020

Clean up ME0MuonTrackCollProducer #31386

Merged

silviodonato mentioned this pull request Nov 26, 2020

DQMOnlineClient-beam_dqm_sourceclient error #31896

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ME0MuonSelector cleanup pass #31190

ME0MuonSelector cleanup pass #31190

davidlange6 commented Aug 20, 2020

cmsbuild commented Aug 20, 2020

cmsbuild commented Aug 20, 2020

cmsbuild commented Aug 20, 2020

davidlange6 commented Aug 20, 2020

cmsbuild commented Aug 20, 2020

guitargeek commented Aug 20, 2020

perrotta commented Aug 20, 2020

perrotta commented Aug 20, 2020

davidlange6 commented Aug 24, 2020

davidlange6 commented Aug 24, 2020

cmsbuild commented Aug 24, 2020 •

edited

cmsbuild commented Aug 24, 2020

cmsbuild commented Aug 24, 2020

davidlange6 commented Aug 24, 2020 via email

davidlange6 commented Aug 24, 2020 via email

cmsbuild commented Aug 24, 2020 •

edited

cmsbuild commented Aug 24, 2020

cmsbuild commented Aug 24, 2020

cmsbuild commented Aug 24, 2020

perrotta commented Sep 2, 2020

perrotta commented Sep 2, 2020

cmsbuild commented Sep 2, 2020

qliphy commented Sep 3, 2020

ME0MuonSelector cleanup pass #31190

ME0MuonSelector cleanup pass #31190

Conversation

davidlange6 commented Aug 20, 2020

cmsbuild commented Aug 20, 2020

cmsbuild commented Aug 20, 2020

cmsbuild commented Aug 20, 2020

davidlange6 commented Aug 20, 2020

cmsbuild commented Aug 20, 2020

guitargeek commented Aug 20, 2020

perrotta commented Aug 20, 2020

perrotta commented Aug 20, 2020

davidlange6 commented Aug 24, 2020

davidlange6 commented Aug 24, 2020

cmsbuild commented Aug 24, 2020 • edited

cmsbuild commented Aug 24, 2020

cmsbuild commented Aug 24, 2020

davidlange6 commented Aug 24, 2020 via email

davidlange6 commented Aug 24, 2020 via email

cmsbuild commented Aug 24, 2020 • edited

cmsbuild commented Aug 24, 2020

cmsbuild commented Aug 24, 2020

cmsbuild commented Aug 24, 2020

perrotta commented Sep 2, 2020

perrotta commented Sep 2, 2020

cmsbuild commented Sep 2, 2020

qliphy commented Sep 3, 2020

cmsbuild commented Aug 24, 2020 •

edited

cmsbuild commented Aug 24, 2020 •

edited