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

Extend and fix RecHitTools #27128

Merged
merged 12 commits into from Jun 24, 2019
Merged

Extend and fix RecHitTools #27128

merged 12 commits into from Jun 24, 2019

Conversation

clelange
Copy link
Contributor

@clelange clelange commented Jun 6, 2019

  • add bhOffset_,
  • fix getLayerWithOffset for new geometry,
  • add firstLayerBH(), lastLayerBH()

PR description:

This PR introduces some important functionality and fixes in/to the RecHitTools required to eventually remove magic numbers in other places of the HGCal reconstruction code.
Related to #26225

PR validation:

Ran some test code with D41 geometry and got the following output:

bhOffset_: 36
EE layers: 28
FH layers: 22
BH layers: 14
first EE layer: 1
first FH layer: 1
first BH layer: 9
EE layer at z = 10000: 28
FH layer at z = 10000: 24
BH layer at z = 10000: 24
lastLayerEE: 28
lastLayerFH: 50
lastLayerBH: 50
firstLayerBH: 37

There are some discrepancies that need to be understood, which, however, seem to be in the geometry, underlining the importance of this PR for geometry validation.
For example: D41 should have 50 HGCal layers only, 22 FH layers, 14 BH layers.
After talking to @bsunanda , the error might also be in my setup and there are no issues in D41 geoemtry. We will investigate, but that's independent of the PR.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 6, 2019

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 6, 2019

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27128/10257

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 6, 2019

A new Pull Request was created by @clelange (Clemens Lange) for master.

It involves the following packages:

RecoLocalCalo/HGCalRecAlgos

@perrotta, @cmsbuild, @kpedro88, @slava77 can you please review it and eventually sign? Thanks.
@edjtscott, @vandreev11, @sethzenz, @felicepantaleo, @rovere, @argiro, @cseez, @pfs, @lgray, @kpedro88 this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@kpedro88
Copy link
Contributor

kpedro88 commented Jun 6, 2019

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 6, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/803/console Started: 2019/06/06 17:46

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 6, 2019

-1

Tested at: c10b31a

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ce1698/803/summary.html

I found follow errors while testing this PR

Failed tests: RelVals

  • RelVals:

When I ran the RelVals I found an error in the following workflows:
27434.0 step3

runTheMatrix-results/27434.0_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D35_GenSimHLBeamSpotFull14+DigiFullTrigger_2023D35+RecoFullGlobal_2023D35+HARVESTFullGlobal_2023D35/step3_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D35_GenSimHLBeamSpotFull14+DigiFullTrigger_2023D35+RecoFullGlobal_2023D35+HARVESTFullGlobal_2023D35.log

29034.0 step3
runTheMatrix-results/29034.0_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D41_GenSimHLBeamSpotFull14+DigiFullTrigger_2023D41+RecoFullGlobal_2023D41+HARVESTFullGlobal_2023D41/step3_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D41_GenSimHLBeamSpotFull14+DigiFullTrigger_2023D41+RecoFullGlobal_2023D41+HARVESTFullGlobal_2023D41.log

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 6, 2019

Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped)

@clelange
Copy link
Contributor Author

clelange commented Jun 6, 2019

OK, that's somewhat expected given the layer numbers I gave above. I'll run those workflows myself to figure out why exactly they fail.

Module: HGCalLayerClusterProducer:hgcalLayerClusters (crashed)

A fatal system signal has occurred: segmentation violation

@cmsbuild
Copy link
Contributor

Pull request #27128 was updated. @perrotta, @cmsbuild, @kpedro88, @slava77 can you please check and sign again.

@kpedro88
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 20, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/1084/console Started: 2019/06/20 22:38

@cmsbuild
Copy link
Contributor

@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-ce1698/1084/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 659 differences found in the comparisons
  • DQMHistoTests: Total files compared: 33
  • DQMHistoTests: Total histograms compared: 3254096
  • DQMHistoTests: Total failures: 73
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3253689
  • DQMHistoTests: Total skipped: 334
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 32 files compared)
  • Checked 137 log files, 14 edm output root files, 33 DQM output files

@perrotta
Copy link
Contributor

+1

  • It allows to centrally retrieve HGCal layer bounds, and fixes offsets for them
  • The changes in the output of jenkis tests show the effect of those fixes in the affected Phase2 geometries, as confirmed in the github thread starting from Extend and fix RecHitTools #27128 (comment)
  • Further differences that showed us during the developing phase of this PR were found to be spurious and fixed with the last commit 68b182e
    • Jenkins tests do not show unexpected differences wrt the baseline any more
  • This PR will likely conflict with TICL Reorganization, reloaded. #27160, which is also supposed to get ready soon

@perrotta
Copy link
Contributor

@kpedro88 please have a look

@kpedro88
Copy link
Contributor

+upgrade

@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. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

@rovere
Copy link
Contributor

rovere commented Jun 24, 2019

Now that all the mandatory signatures are there, it would be nice integrating this asap, so that #27160 could be rebased for the last time, hopefully, and quickly reviewed and integrated itself.
Thanks.

@fabiocos
Copy link
Contributor

@rovere indeed there will be a conflict between this PR and #27160, independent on the relative order of integration. At this point I will integrate it in next IB

@rovere
Copy link
Contributor

rovere commented Jun 24, 2019

@fabiocos Yes, I was suggesting to put this in soon, so that the bigger #27160 could be fixed and reviewed. Thanks for the support in this 'messy' period.

@fabiocos
Copy link
Contributor

+1

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

7 participants