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.

Sign up for GitHub

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

Jump to bottom

Bugfixes in simtrack to GEM/CSC digi and stub matching #30608

Merged
merged 8 commits into from
Jul 21, 2020
Merged

Bugfixes in simtrack to GEM/CSC digi and stub matching #30608

merged 8 commits into from
Jul 21, 2020

Conversation

dildick
Copy link
Contributor

@dildick dildick commented Jul 9, 2020 •
edited
Loading

PR description:

Follow-up of #29830. I found a few more bugs while validating the CCLUT algorithm and the GEM trigger primitives in simulation.

PR validation:

Tested with Run-3 MC samples in various CMSSW_11_2_X pre-releases.

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

N/A

@tahuang1991

@cmsbuild cmsbuild added this to the CMSSW_11_2_X milestone Jul 9, 2020
@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 9, 2020

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 9, 2020

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30608/16867

  • This PR adds an extra 28KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 9, 2020

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 9, 2020

-code-checks

ERROR: Build errors found during clang-tidy run.

gmake: *** [tmp/slc7_amd64_gcc820/Makefile:32: .SCRAM/slc7_amd64_gcc820/timestamps/coral] Error 1
gzip: /build/cmsbld/jenkins/workspace/run-pr-code-checks/CMSSW_11_2_X_2020-07-08-2300/.SCRAM/slc7_amd64_gcc820/ToolCache.db.gz: No such file or directory
Can't call method "initpathvars" on an undefined value at /cvmfs/cms-ib.cern.ch/nweek-02636/share/lcg/SCRAMV1/V2_2_9_pre07/src/SCRAM/SCRAM.pm line 392.
	slc7_amd64_gcc820
--
gmake: *** [tmp/slc7_amd64_gcc820/Makefile:32: .SCRAM/slc7_amd64_gcc820/timestamps/cmsswdata] Error 1
gmake: *** [tmp/slc7_amd64_gcc820/Makefile:32: .SCRAM/slc7_amd64_gcc820/timestamps/codechecker] Error 1
gmake: *** [tmp/slc7_amd64_gcc820/Makefile:32: .SCRAM/slc7_amd64_gcc820/timestamps/csctrackfinderemulation] Error 1
You are trying to build/setup tools for SCRAM_ARCH slc7_amd64_gcc820.
while your project area is currently setup for following SCRAM_ARCH(s):
	slc7_amd64_gcc820
--
gmake: *** [tmp/slc7_amd64_gcc820/Makefile:32: .SCRAM/slc7_amd64_gcc820/timestamps/clhepheader] Error 1

gzip: /build/cmsbld/jenkins/workspace/run-pr-code-checks/CMSSW_11_2_X_2020-07-08-2300/.SCRAM/slc7_amd64_gcc820/ToolCache.db.gz: unexpected end of file
Can't call method "initpathvars" on an undefined value at /cvmfs/cms-ib.cern.ch/nweek-02636/share/lcg/SCRAMV1/V2_2_9_pre07/src/SCRAM/SCRAM.pm line 392.
gmake: *** [tmp/slc7_amd64_gcc820/Makefile:32: .SCRAM/slc7_amd64_gcc820/timestamps/cppunit] Error 1
gmake: *** [tmp/slc7_amd64_gcc820/Makefile:32: .SCRAM/slc7_amd64_gcc820/timestamps/cuda] Error 1
Updating symlinks under external/slc7_amd64_gcc820

Finding a value for CLHEP_BASE:
--
gmake: *** No rule to make target 'code-checks'.
gmake: *** [There are compilation/build errors. Please see the detail log above.] Error 2

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 9, 2020

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 9, 2020

-code-checks

ERROR: Build errors found during clang-tidy run.

gmake: *** [tmp/slc7_amd64_gcc820/Makefile:22: .SCRAM/slc7_amd64_gcc820/MakeData/Tools.mk] Error 5
gmake: Failed to remake makefile 'tmp/slc7_amd64_gcc820/Makefile'.
gmake: *** No rule to make target 'code-checks'.
gmake: *** [There are compilation/build errors. Please see the detail log above.] Error 2

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 9, 2020

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+1
Tested at: 92bf921
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b7ba3a/7913/summary.html
CMSSW: CMSSW_11_2_X_2020-07-13-1100
SCRAM_ARCH: slc7_amd64_gcc820

@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-b7ba3a/7913/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 37
  • DQMHistoTests: Total histograms compared: 2787429
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2787378
  • DQMHistoTests: Total skipped: 50
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
  • Checked 154 log files, 17 edm output root files, 37 DQM output files

@jfernan2
Copy link
Contributor

+1
Despite the important changes in the code, specially in the SimHitMatchers, there are no real changes in Validation plots since modules are not run yet

kpedro88
kpedro88 reviewed Jul 14, 2020
View reviewed changes
Validation/MuonCSCDigis/src/CSCStubMatcher.cc
@@ -137,15 +139,28 @@ void CSCStubMatcher::matchCLCTsToSimTrack(const CSCCLCTDigiCollection& clcts) {
for (const auto& p : comps) {
layer++;
for (const auto& q : p) {
if (verboseCLCT_)
cout << "L" << layer << " " << q << " " << q.getHalfStrip() << " " << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

cout is not allowed, use messagelogger

Validation/MuonCSCDigis/src/CSCStubMatcher.cc
if (clctComp == 65535)
continue;
if (verboseCLCT_) {
std::cout << "\t" << clctComp << " " << endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

cout is not allowed

Validation/MuonCSCDigis/src/CSCStubMatcher.cc
nMatches++;
if (verboseCLCT_) {
cout << "\t\tnMatches " << nMatches << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

cout is not allowed

Validation/MuonCSCDigis/src/CSCStubMatcher.cc
}
}
}
if (verboseCLCT_) {
cout << endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

cout is not allowed

Validation/MuonCSCDigis/src/CSCStubMatcher.cc
cout << "in LCT, getCLCT " << lct.getCLCT() << " getALCT " << lct.getALCT() << endl;

if (verboseLCT_) {
cout << ch_id << " " << ch_id2 << endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

cout is not allowed

Validation/MuonGEMDigis/src/GEMDigiMatcher.cc
if (verboseCoPad_)
cout << "CoPad: was matched! " << endl;
superchamber_to_copads_[superch_id()].push_back(*pad);
cout << "...was matched! " << endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

cout is not allowed

Validation/MuonHits/src/CSCSimHitMatcher.cc
@@ -31,6 +31,12 @@ void CSCSimHitMatcher::match(const SimTrack& track, const SimVertex& vertex) {
// instantiates the track ids and simhits
MuonSimHitMatcher::match(track, vertex);

// hard cut on non-CSC muons
if (std::abs(track.momentum().eta()) < 0.9)
Copy link
Contributor

Choose a reason for hiding this comment

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

magic numbers

Validation/MuonHits/src/GEMSimHitMatcher.cc
@@ -31,6 +31,10 @@ void GEMSimHitMatcher::match(const SimTrack& track, const SimVertex& vertex) {
// instantiates the track ids and simhits
MuonSimHitMatcher::match(track, vertex);

// hard cut on non-GEM muons
if (std::abs(track.momentum().eta()) < 1.55)
Copy link
Contributor

Choose a reason for hiding this comment

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

magic number

Validation/MuonHits/src/GEMSimHitMatcher.cc
const GEMDetId& p_id(h.detUnitId());

if (verbose_)
std::cout << "Candidate GEM simhit " << p_id << " " << h << " " << h.particleType() << " " << h.processType()
Copy link
Contributor

Choose a reason for hiding this comment

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

cout is not allowed

Validation/MuonHits/src/GEMSimHitMatcher.cc
const GEMDetId& p_id(h.detUnitId());
detid_to_hits_[h.detUnitId()].push_back(h);
if (verbose_)
std::cout << "...was matched" << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

cout is not allowed

@dildick
Copy link
Contributor Author

dildick commented Jul 14, 2020

@jfernan2 Thanks. Yes, for the time being, the plugin and analyzers are kept in https://github.com/gem-sw/GEMCode/blob/for-CMSSW_11_1_X/GEMValidation/plugins/GEMCSCAnalyzer.cc and https://github.com/gem-sw/GEMCode/tree/for-CMSSW_11_1_X/GEMValidation/interface/Analyzers. I want to move part of it to CMSSW, so that people can do GEM & CSC TP performance studies relatively quickly.

@kpedro88
Copy link
Contributor

+upgrade
realized my comments were all in Validation subsystem, which is outside of the upgrade category
(would still be good to address them, but they apply equally to the code as it existed before this PR)

@jfernan2
Copy link
Contributor

@dildick could you please implement the changes requested by @kpedro88 in the next iteration of the GEM/CSC Validation?
Thanks

@dildick
Copy link
Contributor Author

dildick commented Jul 15, 2020

@jfernan2 Sure. I can do that.

@civanch
Copy link
Contributor

civanch commented Jul 21, 2020

+1

@cmsbuild
Copy link
Contributor

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 (and backports should be raised in the release meeting by the corresponding L2)

@silviodonato
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit e61db3c into cms-sw:master Jul 21, 2020
@cmsbuild cmsbuild mentioned this pull request Jul 21, 2020
Closed
@dildick dildick deleted the from-CMSSW_11_2_X_2020-07-08-1100-GEM-CSC-digi-stub-matching branch July 28, 2020 19:20
@dildick dildick mentioned this pull request Sep 23, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kpedro88 kpedro88 kpedro88 left review comments

Assignees
No one assigned
Labels
code-checks-approved comparison-available dqm-approved fully-signed orp-approved simulation-approved tests-approved upgrade-approved
Projects
None yet
Milestone
CMSSW_11_2_X
Development

Successfully merging this pull request may close these issues.
6 participants
@dildick @cmsbuild @jfernan2 @kpedro88 @civanch @silviodonato