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

MTD geometry: porting of MTDTopology to DD4hep #29599

Merged
merged 4 commits into from May 5, 2020

Conversation

fabiocos
Copy link
Contributor

PR description:

In this PR the existing code describing MTD topology is updated to support DD4hep, both for producer and test analyzer.

PR validation:

The MTDTopologyAnalyzer test runs and provide the same results for DD4hep as for DDD on scenarios D50 and D53. As the MTD reco numbering code porting to DD4hep is still under debugging, the test is at present a reduced version of the final one. No impact on production workflows should be expected.

@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/cms-sw-PR-29599/14934

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @fabiocos (Fabio Cossutti) for master.

It involves the following packages:

Geometry/MTDGeometryBuilder
Geometry/MTDNumberingBuilder

@civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @kpedro88 can you please review it and eventually sign? Thanks.
@silviodonato, @dpiparo you are the release manager for this.

cms-bot commands are listed here

@fabiocos
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 30, 2020

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/5930/console Started: 2020/04/30 13:28

@cmsbuild
Copy link
Contributor

+1
Tested at: bd9dbd0
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b61b42/5930/summary.html
CMSSW: CMSSW_11_1_X_2020-04-29-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-b61b42/5930/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: 2696435
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2696115
  • 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

@cvuosalo
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented May 5, 2020

+1
Tested at: a94cc59
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b61b42/6004/summary.html
CMSSW: CMSSW_11_1_X_2020-05-04-2300
SCRAM_ARCH: slc7_amd64_gcc820

@cmsbuild
Copy link
Contributor

cmsbuild commented May 5, 2020

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 5, 2020

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b61b42/6004/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: 2696239
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2695919
  • 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

@silviodonato
Copy link
Contributor

@cvuosalo could you confirm your signature?
@kpedro88 does the last version of the code address your comments?

@kpedro88
Copy link
Contributor

kpedro88 commented May 5, 2020

+upgrade

@silviodonato
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit 3297972 into cms-sw:master May 5, 2020
} else {
dd4hepToken_ = cc.consumesFrom<cms::DDCompactView, IdealGeometryRecord>(edm::ESInputTag());
}
edm::LogInfo("MTDParametersESModule") << "MTDParametersESModule::MTDParametersESModule";
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noticing this line. Shouldn't it be LogTrace?

Copy link
Contributor

Choose a reason for hiding this comment

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

Throughout this PR, shouldn't the LogInfo/LogVerbatim be changed to LogTrace?

This PR has already been merged, so these changes would have to go in a later PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cvuosalo this is a minimal update of older code, I agree with you that it might be set as optional (even though it is not so far). Anyway 1) Info verbosity is not used in production 2) LogDebug/LogTrace management looks to me quite impractical and cumbersome and 3) it is basically not tested (I have recently made a cleanup PR for many LogDebug lines not even compiling when invoked, because they are generally not probed in regular PR tests). I can move there printouts under EDM_ML_DEBUG flag control as usual in #29647

Copy link
Contributor

Choose a reason for hiding this comment

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

it is basically not tested (I have recently made a cleanup PR for many LogDebug lines not even compiling when invoked, because they are generally not probed in regular PR tests). I can move there printouts under EDM_ML_DEBUG flag control as usual in #29647h

Code under EDM_ML_DEBUG has the same risks towards bitrot as LogDebug, so the only benefit of not using something else than LogDebug inside #ifdef EDM_ML_DEBUG is possibly a bit easier configuration of the MessageLogger (LogInfo needs some as well).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@makortel in this respect you are correct, the simpler update of the default MessageLogger configuration to switch on/off messages is the main reason for this choice of mine (in general we want to comment the declarations of EDM_ML_DEBUG in the code, so my comment 3 is not really relevant in that case). In #29250 my suggestion was to implement a check of the output of compilation with that flag, enabled for the static analyzer, which I understand has been implemented by @smuzaffar in cms-sw/cms-bot#1289

@cvuosalo
Copy link
Contributor

cvuosalo commented May 5, 2020

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented May 5, 2020

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 be automatically merged.

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