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

CSC Upgrade TMB base classes and improved LUT generator code #18560

Merged

cmsbuild merged 18 commits into cms-sw:master from dildick:from-CMSSW_9_1_X_2017-04-28-1100-add-tmb-base-class

Jun 22, 2017

Contributor

dildick commented May 4, 2017 •

edited

This PR is a follow up of #17843 where I introduced a first working version of a LUT generator class.

This PR extends and improves that class (CSCUpgradeMotherboardLUTGenerator). The actual LUTs are stored in CSCUpgradeMotherboardLUT. Most LUTs are currently empty but will be filled soon.
This PR also introduces base classes for the upgrade TMBs: (1) CSCUpgradeMotherboard which is a common class for all upgrade CSC-GEM and CSC-RPC TMBs and (2) CSCGEMMotherboard which is derived from CSCUpgradeMotherboard. The latter will form the base class for the specific classes CSCGEMMotherboardME11, CSCGEMMotherboardME21.
In a follow up pull request I will change the names of CSCMotherboardME11GEM and CSCMotherboardME21GEM to CSCGEMMotherboardME11 and CSCGEMMotherboardME21 respectively, and I will let them derive from CSCGEMMotherboard. CSCMotherboardME3141RPC will be renamed to CSCRPCMotherboard and will derive from CSCUpgradeMotherboard.
No changes with respect to the run-2 or phase-2 work flows are expected, since none of this code is enabled yet.

cmsbuild added this to the CMSSW_9_1_X milestone

cmsbuild added comparison-pending l1-pending orp-pending pending-signatures tests-pending labels

Contributor

cmsbuild commented May 4, 2017

A new Pull Request was created by @dildick (Sven Dildick) for master.

It involves the following packages:

L1Trigger/CSCTriggerPrimitives

@cmsbuild, @rekovic, @mulhearn, @davidlange6 can you please review it and eventually sign? Thanks.
@valuev, @ptcox, @kreczko, @Martin-Grunewald this is something you requested to watch as well.
@davidlange6 you are the release manager for this.

cms-bot commands are listed here

Contributor Author

dildick commented May 4, 2017

smuzaffar modified the milestones: CMSSW_9_2_X, CMSSW_9_1_X

Contributor

kpedro88 commented May 4, 2017

assign upgrade

Contributor

cmsbuild commented May 4, 2017

New categories assigned: upgrade

@kpedro88 you have been requested to review this Pull request/Issue and eventually sign? Thanks

cmsbuild added the upgrade-pending label

kpedro88 reviewed

View reviewed changes

Contributor

kpedro88 left a comment

Main issues are proper use of const references with auto and smart pointers instead of bare pointers

L1Trigger/CSCTriggerPrimitives/src/CSCGEMMotherboard.cc Outdated

+                coPadProcessor.reset( new GEMCoPadProcessor(endcap, station, 1, chamber, coPadParams) );
+                // use "old" or "new" dataformat for integrated LCTs?
+                useOldLCTDataFormat_ = tmbParams_.getParameter<bool>("useOldLCTDataFormat");

Contributor

kpedro88 May 4, 2017

Most of these member variable assignments could go in the constructor initialization list

Contributor Author

dildick May 4, 2017

ok. will do

L1Trigger/CSCTriggerPrimitives/src/CSCGEMMotherboard.cc Outdated

+              void CSCGEMMotherboard::retrieveGEMPads(const GEMPadDigiCollection* gemPads, unsigned id)
+              {
+                auto superChamber(gem_g->superChamber(id));
+                for (auto ch : superChamber->chambers()) {

Contributor

kpedro88 May 4, 2017

All of these loop variables should be const auto& (unless there's a good reason to make it non-const or a copy)

Contributor Author

dildick May 4, 2017

ok. will do

L1Trigger/CSCTriggerPrimitives/src/CSCGEMMotherboard.cc Outdated

+              void CSCGEMMotherboard::retrieveGEMCoPads()
+              {
+                for (auto copad: gemCoPadV){

Contributor

kpedro88 May 4, 2017

again, const auto&

Contributor Author

dildick May 4, 2017

ok. will do

L1Trigger/CSCTriggerPrimitives/src/CSCGEMMotherboard.cc Outdated

+               							 enum CSCPart part,
+              							 int trknmb)
+              {
+                auto mymap = (*getLUT()->get_gem_pad_to_csc_hs(par, part));

Contributor

kpedro88 May 4, 2017

const auto&

Contributor Author

dildick May 4, 2017

ok. will do

L1Trigger/CSCTriggerPrimitives/src/CSCGEMMotherboard.cc Outdated

+                  // get wiregroup from ALCT
+                  int wg = alct.getKeyWG();
+                  // if (keyStrip>wgvshs[wg][0] && keyStrip<wgvshs[wg][1])

Contributor

kpedro88 May 4, 2017

Remove commented-out code

Contributor Author

dildick May 4, 2017

ok. will do

L1Trigger/CSCTriggerPrimitives/src/CSCUpgradeMotherboardLUTGenerator.cc Outdated

+              int CSCUpgradeMotherboardLUTGenerator::assignRoll(const std::vector<std::pair<double,double> >& lut_, double eta) const
+              {
+                int result = -99;
+                for(auto p : lut_) {

Contributor

kpedro88 May 4, 2017

const auto&

L1Trigger/CSCTriggerPrimitives/src/CSCUpgradeMotherboardLUTGenerator.cc Outdated

+              CSCUpgradeMotherboardLUTGenerator::gemRollToEtaLimitsLUT(const GEMChamber* gemChamber) const
+              {
+                std::vector<std::pair<double,double> > lut;
+                for(auto roll : gemChamber->etaPartitions()) {

Contributor

kpedro88 May 4, 2017

const auto&

L1Trigger/CSCTriggerPrimitives/src/CSCUpgradeMotherboardLUTGenerator.cc Outdated

+              CSCUpgradeMotherboardLUTGenerator::rpcRollToEtaLimitsLUT(const RPCChamber* rpcChamber) const
+              {
+                std::vector<std::pair<double,double> > lut;
+                for(auto roll : rpcChamber->rolls()) {

Contributor

kpedro88 May 4, 2017

const auto&

L1Trigger/CSCTriggerPrimitives/src/CSCUpgradeMotherboardLUTGenerator.h Outdated

+              {
+                int i = 0;
+                os << "{" << std::endl;
+                for(auto p : v) {

Contributor

kpedro88 May 4, 2017

const auto&

L1Trigger/CSCTriggerPrimitives/src/CSCUpgradeMotherboardLUTGenerator.h Outdated

+              {
+                int i = 0;
+                os << "{" << std::endl;
+                for(auto p : v) {

Contributor

kpedro88 May 4, 2017

const auto&

Contributor

kpedro88 commented May 4, 2017

@dildick no need to reply to comments individually unless you have an objection

cmsbuild added alca-pending db-pending hlt-pending reconstruction-pending simulation-pending labels

Contributor

cmsbuild commented May 8, 2017

Pull request #18560 was updated. @perrotta, @cmsbuild, @civanch, @ghellwig, @silviodonato, @arunhep, @mdhildreth, @Martin-Grunewald, @rekovic, @franzoni, @kpedro88, @cerminar, @slava77, @ggovi, @fwyzard, @mmusich, @mulhearn, @davidlange6 can you please check and sign again.

Contributor Author

dildick commented May 8, 2017

I will rebase.

Contributor

kpedro88 commented May 8, 2017

-1
merge is not rebase

cmsbuild added upgrade-rejected upgrade-pending and removed upgrade-pending upgrade-rejected labels

Contributor

cmsbuild commented May 8, 2017

Pull request #18560 was updated. @perrotta, @cmsbuild, @civanch, @ghellwig, @silviodonato, @arunhep, @mdhildreth, @Martin-Grunewald, @rekovic, @franzoni, @kpedro88, @cerminar, @slava77, @ggovi, @fwyzard, @mmusich, @mulhearn, @davidlange6 can you please check and sign again.

Contributor

kpedro88 commented May 8, 2017

-1
maybe the base is wrong? master = 9_2_X, not 9_1_X

cmsbuild removed the upgrade-pending label

Sven Dildick added 4 commits

June 19, 2017 23:34


          reduce code duplication

4be5822


          suggestions by David Lange

d5740ce


          helper function

d28ad28


          suggestions by David Lange

f82f46b

Contributor

cmsbuild commented Jun 19, 2017

Pull request #18560 was updated. @cmsbuild, @rekovic, @mulhearn, @kpedro88, @davidlange6 can you please check and sign again.


          suggestions by David Lange

766476e

Contributor Author

dildick commented Jun 19, 2017

@kpedro88 I made template specializations where possible. For other occurrences of "is_same" I implemented different solutions. In any case, the is_same constructs have been removed.

Contributor

cmsbuild commented Jun 19, 2017

Pull request #18560 was updated. @cmsbuild, @rekovic, @mulhearn, @kpedro88, @davidlange6 can you please check and sign again.

Contributor

kpedro88 commented Jun 19, 2017

please test

Contributor

cmsbuild commented Jun 19, 2017 •

edited

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

cmsbuild added tests-started and removed tests-pending labels

Contributor

cmsbuild commented Jun 19, 2017

+1
Tested at: 766476e
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18560/20770/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
d7c0a73
490b8be
35377aa
5b2c111
de7a19e
a276f11
483bae8
ffecb3b
648186f
a6aa2e4
0a97710
70499c2
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18560/20770/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18560/20770/git-merge-result

Contributor

cmsbuild commented Jun 19, 2017

Comparison job queued.

cmsbuild added tests-approved and removed tests-started labels

Contributor

cmsbuild commented Jun 20, 2017

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 61 differences found in the comparisons
DQMHistoTests: Total files compared: 22
DQMHistoTests: Total histograms compared: 1802826
DQMHistoTests: Total failures: 29662
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 1772998
DQMHistoTests: Total skipped: 166
DQMHistoTests: Total Missing objects: 0
Checked 90 log files, 14 edm output root files, 22 DQM output files

cmsbuild added comparison-available and removed comparison-pending labels

Contributor

kpedro88 commented Jun 20, 2017

+1

cmsbuild added upgrade-approved and removed upgrade-pending labels

Contributor

davidlange6 commented Jun 22, 2017

merge

cmsbuild merged commit 4bfcf97 into cms-sw:master

Contributor

smuzaffar commented Jun 22, 2017

@dildick , can you please provide a fix to avoid these compilation warnings
https://cmssdt.cern.ch/SDT/cgi-bin/buildlogs/slc6_amd64_gcc530/CMSSW_9_2_X_2017-06-22-1000/L1Trigger/CSCTriggerPrimitives

Contributor Author

dildick commented Jun 22, 2017

@smuzaffar No problem. That will be resolved in a next PR.

Contributor

mrodozov commented Jun 23, 2017

@dildick compile warnings are still there https://cmssdt.cern.ch/SDT/cgi-bin/buildlogs/slc7_amd64_gcc630/CMSSW_9_2_X_2017-06-23-1100/L1Trigger/CSCTriggerPrimitives
would you please fix this for the next IB ?

Contributor Author

dildick commented Jun 23, 2017 •

edited

@smuzaffar @mrodozov #19418

dildick deleted the from-CMSSW_9_1_X_2017-04-28-1100-add-tmb-base-class branch

June 23, 2017 16:03

dildick mentioned this pull request

GEM-CSC local trigger: make ME11 and ME21 TMBs inherit from same base class #20509

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment