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

Add possibility to specify GBT Id only to DTC cabling map producer #29443

Merged

Conversation

luigicalligaris
Copy link

@luigicalligaris luigicalligaris commented Apr 9, 2020

Change the PSet parameters in the DTCCablingMapProducer, so the user can choose to provide the elink and gbt ids, provide the gbt id and generate a dummy sequential elink id, or generate both

PR description:

Following feature request 29390 (#29390) the producer code was changed, such that the user can choose three operating modes:

  • gbtlink id and elink id are provided in the csv
  • only gbtlink id is provided in the csv, the elink id is sequentially assigned dummy values to detectors as they are encountered in the same gbt id
  • both gbtlink id and elink id are sequentially assigned with dummy values

The parameter choosing the mode is implemented as a string, and the corresponding state flag is implemented as an int type (enum) for performance reasons.

Due to the increased risk of the user choosing the wrong mode or column configuration, and due to the cryptic message resulting from an out-of-bound access to a vector when using operator[], bound checks have been activated replacing operator[] with at()

No other changes to the code were introduced, and the formatting style was kept the same.
No dependence on external PRs.

PR validation:

Validated on the DTC cabling map payload production, retrieve and dump PSets

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

Not a backport.

…can choose to provide the elink and gbt ids, provide the gbt id and generate a dummy sequential elink id, or generate both
@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 9, 2020

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 9, 2020

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29443/14610

  • This PR adds an extra 16KB to repository

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

else {
std::ostringstream message;
message << "Parameter dummy_fill_mode with invalid value: " << dummy_fill_mode_param;
throw cms::Exception(message.str());
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be:

throw cms::Exception("InvalidDummyFillMode") << "Parameter dummy_fill_mode with invalid value: " << dummy_fill_mode_param;

the value used in the exception constructor is the category, not the message.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed, also in other parts of the code, to specify the category properly.

@kpedro88
Copy link
Contributor

kpedro88 commented Apr 9, 2020

@luigicalligaris in addition to the review comment:

  • please apply the automatic code format (otherwise the tests cannot be started)
  • edit the PR description to link to the appropriate issue correctly (currently there is a typo in the link)

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 9, 2020

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 9, 2020

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29443/14613

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 9, 2020

A new Pull Request was created by @luigicalligaris (Luigi Calligaris) for master.

It involves the following packages:

CondTools/SiPhase2Tracker

@christopheralanwest, @kpedro88, @cmsbuild, @tocheng, @tlampen, @pohsun can you please review it and eventually sign? Thanks.
@mmusich, @tocheng, @VinInn this is something you requested to watch as well.
@silviodonato, @dpiparo you are the release manager for this.

cms-bot commands are listed here

@luigicalligaris
Copy link
Author

@luigicalligaris in addition to the review comment:

  • please apply the automatic code format (otherwise the tests cannot be started)
  • edit the PR description to link to the appropriate issue correctly (currently there is a typo in the link)
  • applied auto code fromat from bot
  • removed excess "0" in link in description, tested working

@luigicalligaris
Copy link
Author

When good for you, ready to squash

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29443/14648

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

Pull request #29443 was updated. @christopheralanwest, @kpedro88, @cmsbuild, @tocheng, @tlampen, @pohsun can you please check and sign again.

@pohsun
Copy link

pohsun commented Apr 11, 2020

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 11, 2020

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/5668/console Started: 2020/04/11 14:38

@cmsbuild
Copy link
Contributor

+1
Tested at: 4516b93
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b1a2db/5668/summary.html
CMSSW: CMSSW_11_1_X_2020-04-10-2300
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-b1a2db/5668/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2695166
  • DQMHistoTests: Total failures: 23
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2694824
  • DQMHistoTests: Total skipped: 319
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
  • Checked 147 log files, 16 edm output root files, 34 DQM output files

@kpedro88
Copy link
Contributor

+upgrade

@silviodonato
Copy link
Contributor

alca: @pohsun @tlampen @tocheng @christopheralanwest
do you have comments?

@silviodonato
Copy link
Contributor

silviodonato commented Apr 17, 2020

any objection from generators side (@alberto-sanchez @agrohsje @efeyazgan @mkirsano @qliphy @SiewYan) ?

@pohsun
Copy link

pohsun commented Apr 19, 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 86965c1 into cms-sw:master Apr 20, 2020
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

6 participants