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: DD4hep migration, preliminary update and cleaning of ideal geometry test #29407

Merged
merged 9 commits into from Apr 10, 2020

Conversation

fabiocos
Copy link
Contributor

@fabiocos fabiocos commented Apr 7, 2020

PR description:

This PR updates DD4hep scenarios D50 and D53 according to the most recent updates, and refresh the MTDCommonData tests. With the opportunity a few constants for the scenario D53 are added to ETLDetId and ETLNumberingScheme, and the MtdSD test verbosity is improved. While preparing the migration of MTDNumberingBuilder (to come in a next PR), I realized that even the old basic DD4hep geometry test had occasional problems with the printed strings, but no sign of issue with the computed DetIds or positions (which makes me think this is mostly a printout issue, although I do not fully understand it).
I have refreshed and simplified the test to avoid useless code duplication and multiple output streams, getting the same results.

PR validation:

The standalone MTDCommonData tests run and produce consistent output.

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 7, 2020

The code-checks are being triggered in jenkins.

@fabiocos
Copy link
Contributor Author

fabiocos commented Apr 7, 2020

@cvuosalo @ianna this PR (and the next one) would benefit from #29387 for the geoHistory computation

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 7, 2020

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29407/14558

  • This PR adds an extra 32KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 7, 2020

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

It involves the following packages:

DataFormats/ForwardDetId
Geometry/CMSCommonData
Geometry/MTDCommonData
SimG4CMS/Forward

@civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @kpedro88 can you please review it and eventually sign? Thanks.
@makortel, @apsallid, @rovere this is something you requested to watch as well.
@silviodonato, @dpiparo you are the release manager for this.

cms-bot commands are listed here

@fabiocos
Copy link
Contributor Author

fabiocos commented Apr 7, 2020

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 7, 2020

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/5577/console Started: 2020/04/07 21:01

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 7, 2020

+1
Tested at: 9ff4f63
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d54517/5577/summary.html
CMSSW: CMSSW_11_1_X_2020-04-07-1100
SCRAM_ARCH: slc7_amd64_gcc820

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 7, 2020

Comparison job queued.

@civanch
Copy link
Contributor

civanch commented Apr 9, 2020

+1

@fabiocos , since some moment, we are trying use LogVerbatim instead of LogInfo, because in the last variant log files are polluted by date and time at each printout, which make a problem to use diff/tkdiff

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 9, 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 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)

@fabiocos
Copy link
Contributor Author

fabiocos commented Apr 9, 2020

@civanch in general I agree about LogVerbatim vs LogInfo. Anyway 1) several statements are under the debug flag EDM_ML_DEBUG , so no affecting normally anything, and the test code is never run in default workflows, but always in standalone tests where I do want to have the output. As you see I have tried to avoid multiple streams, i.e. both MessageLogger and external files as in the DDD tests, because I notice some issues with printing names coming from DD4hep that I am not fully understanding, but I am afraid might be made worse by multiple streams. And in that case I use different labels of messages to collect them in files I can directly compare with the old ones, I have a python script to parse MessageLogger output that can selectively do this (may be I should make it available).

@silviodonato
Copy link
Contributor

please test workflow 23434.1001

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 9, 2020

The tests are being triggered in jenkins.
Test Parameters:

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 9, 2020

+1
Tested at: 9ff4f63
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d54517/5637/summary.html
CMSSW: CMSSW_11_1_X_2020-04-09-1100
SCRAM_ARCH: slc7_amd64_gcc820

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 9, 2020

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 9, 2020

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

There are some workflows for which there are errors in the baseline:
20034.0 step 3
20434.0 step 3
23234.0 step 3
21234.0 step 3
The results for the comparisons for these workflows could be incomplete
This means most likely that the IB is having errors in the relvals.The error does NOT come from this pull request

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-d54517/23434.1001_TTbar_14TeV+RecoFullGlobalPU_TestOldDigi_2026D49PU+HARVESTFullGlobalPU_2026D49PU

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 30
  • DQMHistoTests: Total histograms compared: 2348370
  • DQMHistoTests: Total failures: 3
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2348092
  • DQMHistoTests: Total skipped: 275
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 29 files compared)
  • Checked 143 log files, 16 edm output root files, 30 DQM output files

@fabiocos
Copy link
Contributor Author

@silviodonato the problems in the IB need to be solved, before a meaningful test for this PR can be made

@silviodonato
Copy link
Contributor

+1

23434.1001_TTbar_14TeV+RecoFullGlobalPU_TestOldDigi_2026D49PU+HARVESTFullGlobalPU_2026D49PU Step0-PASSED Step1-PASSED - time date Thu Apr 9 23:29:03 2020-date Thu Apr 9 22:30:05 2020; exit: 0 0

@cmsbuild cmsbuild merged commit fbddf57 into cms-sw:master Apr 10, 2020
@fabiocos fabiocos deleted the fc-mtddd4hep1 branch April 14, 2020 08:19
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