GPU refactor common code from ECAL multifit and HCAL mahi #32144

mariadalfonso · 2020-11-15T15:23:57Z

move here function used both in
RecoLocalCalo/EcalRecProducers/plugins/AmplitudeComputationKernels.cu
and
RecoLocalCalo/HcalRecProducers/src/MahiGPU.cu

@fwyzard

cmsbuild · 2020-11-15T15:32:07Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32144/19816

This PR adds an extra 16KB to repository

cmsbuild · 2020-11-15T15:32:29Z

A new Pull Request was created by @mariadalfonso for master.

It involves the following packages:

DataFormats/CaloRecHit

@perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@rovere this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

fwyzard · 2020-11-15T20:58:56Z

please test

cmsbuild · 2020-11-15T20:59:21Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-11-15-0000/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/10764/console Started: 2020/11/15 22:01

cmsbuild · 2020-11-15T22:22:22Z

+1
Tested at: 5bef34a
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d5fc7e/10764/summary.html
CMSSW: CMSSW_11_2_X_2020-11-15-0000
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-11-15T22:22:24Z

Comparison job queued.

cmsbuild · 2020-11-15T23:38:15Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 2 differences found in the comparisons
DQMHistoTests: Total files compared: 35
DQMHistoTests: Total histograms compared: 2529296
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2529267
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.004 KiB( 34 files compared)
DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
Checked 148 log files, 22 edm output root files, 35 DQM output files

fwyzard · 2020-11-16T00:09:17Z

@mariadalfonso could you change the title of the PR to be a bit more descriptive ?
For example

Refactor common code from ECAL multifit and HCAL mahi

@jpata @perrotta I can rebase #31719 and #31720 and remove the common code once this is merged.

davidlange6 · 2020-11-16T08:55:32Z

Hi @mariadalfonso - dependency-wise, what drives this to go to DataFormats? (its clearly not a data format but an algorithm). Could it instead go to a package like RecoLocalCalo/CommonAlgos (would be new) or CommonTools/RecoAlgos (if the usage is to grow beyond RecoLocalCalo)?

fwyzard · 2020-11-16T09:19:14Z

Hi @davidlange6, the use of DataFormats/CaloRecHit was introduced by #31704 becase DataFormats/Math was deemed too generic for calorimeter code.

If necessary, things can be moved again of course, but I would agree to that only after all PRs depending on it have been merged.

jpata · 2020-11-16T12:46:55Z

Could it instead go to a package like RecoLocalCalo/CommonAlgos (would be new) or CommonTools/RecoAlgos (if the usage is to grow beyond RecoLocalCalo)?
If necessary, things can be moved again of course, but I would agree to that only after all PRs depending on it have been merged.

The minimal suggestion to move this from DataFormats/Math -> DataFormats/CaloRecHit was to discourage this apparently calorimeter-specific code to be relied on by others for generic math operations. I agree that as it's not a dataformat as such, it would have a better home in some non-DataFormats package - thx for the suggestion.

I can rebase #31719 and #31720 and remove the common code once this is merged.

Sounds good to me.

cmsbuild · 2020-11-16T14:47:08Z

The tests are being triggered in jenkins.
Tested with other pull request(s) #31719
Test Parameters:

ENABLE_BOT_TESTS = GPU
CMSSW_11_2_X_2020-11-15-2300/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/10784/console Started: 2020/11/16 15:47
CMSSW_11_2_X_2020-11-15-2300/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/10785/console Started: 2020/11/16 14:54

cmsbuild · 2020-11-16T17:58:47Z

+1
Tested at: 2554dfe
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d5fc7e/10791/summary.html
CMSSW: CMSSW_11_2_X_2020-11-16-1100
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-11-16T17:58:50Z

Comparison job queued.

cmsbuild · 2020-11-16T17:58:56Z

+1
Tested at: 2554dfe
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d5fc7e/10792/summary.html
CMSSW: CMSSW_11_2_X_2020-11-16-1100
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-11-16T17:58:58Z

Comparison job queued.

cmsbuild · 2020-11-16T19:15:54Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 2 differences found in the comparisons
DQMHistoTests: Total files compared: 35
DQMHistoTests: Total histograms compared: 2529296
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2529273
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
Checked 148 log files, 22 edm output root files, 35 DQM output files

jpata · 2020-11-17T09:19:32Z

Per the discussion in #31735, there is some motivation to reconsider the use of Eigen dependencies in DataFormats, and thus moving the common calo methods as David has suggested. I would not set this as a blocker for this PR necessarily (considering the timeline of the patatrack integration effort), but to be kept in mind as an additional data point.

Although no new unit tests were added for the new functions in this PR (see #31880), it would be good to make sure the existing unit tests at least are running.

It seems like with enable gpu, the CUDA unit tests are not being run on the jenkins infrastructure. @smuzaffar could you comment if this would be possible?

I was trying instead to run the test on my local GPU machine as I have done successfully in the past for the previous patatrack PR #31704 (comment), however, the latest cuda update in CMSSW cms-sw/cmsdist@e412958 requires driver updates which I'm addressing on my infrastructure.

smuzaffar · 2020-11-17T09:25:06Z

@jpata, Yes this is on my to do list but will take some time (hopefully by the end of the year)

jpata · 2020-11-17T09:28:05Z

Yes this is on my to do list but will take some time (hopefully by the end of the year)

OK, thank you for confirming (also that I didn't misunderstand whether/how this is supposed to work)!

Meanwhile, I was able to run the existing unit test successfully at least on my machine:

$ ./test/slc7_amd64_gcc820/test_calo_rechit 
0, 10 GeV, 1 ns  flags=0x0  aux=0x0 
0, 10 GeV, 1 ns  flags=0x0  aux=0x0 
all good!

I have no further comments from the reco side, modulo the Eigen-in-Dataformats / package location issue.

mariadalfonso · 2020-11-17T09:53:16Z

@fwyzard wrote
If necessary, things can be moved again of course, but I would agree to that only after all PRs depending on it have been merged.

@jpata
is this fine ?
When is Ecal/Hcal is all integrated/reviewed it will be more clear what is pure Eigen algebra and caloMultifit algorithm and how to keep in synch c++/gpu code and what is cuda specific or more general

fwyzard · 2020-11-17T10:11:05Z

I have no further comments from the reco side, modulo the Eigen-in-Dataformats / package location issue.

To keep track of this and other requests, I would suggest to create an issue on GitHub to collect all the changes that should be made after the initial PRs have been merged.

jpata · 2020-11-17T11:49:48Z

I opened an issue for this #32159, from my side, I would be OK to move forward like this. I'll wait for comments before proceeding with the reco signature.

fwyzard · 2020-11-17T17:34:41Z

@jpata, could you sign this for RECO to have it merged and allow testing more properly the ECAL and HCAL PRs ?

jpata · 2020-11-17T17:51:04Z

+reconstruction

adds common code to DataFormats/CaloRecHit needed for patatrack PRs Patatrack integration - ECAL local reconstruction (7/N) #31719, Patatrack integration - HCAL local reconstruction (8/N) #31720
existing tests pass as expected (new code is not included in tests)
possibly this package should be moved later, see Relocate algos from DataFormats/CaloRecHit, clean up for coding rules #32159
it was pointed out that this code contains a number of code rule violations already from Patatrack integration - calorimeters shared code (6/N) #31704 (single character variables, uppercase variable names), I added this to the issue.

cmsbuild · 2020-11-17T17:51:30Z

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

silviodonato · 2020-11-17T18:01:38Z

+1

move calculateChiSq

5bef34a

cmsbuild added this to the CMSSW_11_2_X milestone Nov 15, 2020

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Nov 15, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 15, 2020

cmsbuild added tests-started and removed tests-pending labels Nov 15, 2020

cmsbuild added tests-approved and removed tests-started labels Nov 15, 2020

cmsbuild added comparison-available and removed comparison-pending labels Nov 15, 2020

mariadalfonso changed the title ~~GPU calocommon~~ GPU refactor common code from ECAL multifit and HCAL mahi Nov 16, 2020

fwyzard mentioned this pull request Nov 16, 2020

Patatrack integration - ECAL local reconstruction (7/N) #31719

Merged

cmsbuild added the tests-started label Nov 16, 2020

cmsbuild added tests-approved and removed tests-started labels Nov 16, 2020

cmsbuild added comparison-available and removed comparison-pending labels Nov 16, 2020

jpata mentioned this pull request Nov 17, 2020

unit tests for patatrack integration: MultifitComputations #31880

Open

jpata mentioned this pull request Nov 17, 2020

Relocate algos from DataFormats/CaloRecHit, clean up for coding rules #32159

Open

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Nov 17, 2020

cmsbuild added orp-approved and removed orp-pending labels Nov 17, 2020

cmsbuild merged commit 6ac73d3 into cms-sw:master Nov 17, 2020

mariadalfonso mentioned this pull request Dec 8, 2020

Open issues regarding the HCAL local reconstruction on GPU #32413

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GPU refactor common code from ECAL multifit and HCAL mahi #32144

GPU refactor common code from ECAL multifit and HCAL mahi #32144

mariadalfonso commented Nov 15, 2020 •

edited

cmsbuild commented Nov 15, 2020

cmsbuild commented Nov 15, 2020

fwyzard commented Nov 15, 2020

cmsbuild commented Nov 15, 2020 •

edited

cmsbuild commented Nov 15, 2020

cmsbuild commented Nov 15, 2020

cmsbuild commented Nov 15, 2020

fwyzard commented Nov 16, 2020

davidlange6 commented Nov 16, 2020

fwyzard commented Nov 16, 2020

jpata commented Nov 16, 2020

cmsbuild commented Nov 16, 2020 •

edited

cmsbuild commented Nov 16, 2020

cmsbuild commented Nov 16, 2020

cmsbuild commented Nov 16, 2020

cmsbuild commented Nov 16, 2020

cmsbuild commented Nov 16, 2020

jpata commented Nov 17, 2020

smuzaffar commented Nov 17, 2020

jpata commented Nov 17, 2020

mariadalfonso commented Nov 17, 2020

fwyzard commented Nov 17, 2020

jpata commented Nov 17, 2020

fwyzard commented Nov 17, 2020

jpata commented Nov 17, 2020

cmsbuild commented Nov 17, 2020

silviodonato commented Nov 17, 2020

GPU refactor common code from ECAL multifit and HCAL mahi #32144

GPU refactor common code from ECAL multifit and HCAL mahi #32144

Conversation

mariadalfonso commented Nov 15, 2020 • edited

cmsbuild commented Nov 15, 2020

cmsbuild commented Nov 15, 2020

fwyzard commented Nov 15, 2020

cmsbuild commented Nov 15, 2020 • edited

cmsbuild commented Nov 15, 2020

cmsbuild commented Nov 15, 2020

cmsbuild commented Nov 15, 2020

fwyzard commented Nov 16, 2020

davidlange6 commented Nov 16, 2020

fwyzard commented Nov 16, 2020

jpata commented Nov 16, 2020

cmsbuild commented Nov 16, 2020 • edited

cmsbuild commented Nov 16, 2020

cmsbuild commented Nov 16, 2020

cmsbuild commented Nov 16, 2020

cmsbuild commented Nov 16, 2020

cmsbuild commented Nov 16, 2020

jpata commented Nov 17, 2020

smuzaffar commented Nov 17, 2020

jpata commented Nov 17, 2020

mariadalfonso commented Nov 17, 2020

fwyzard commented Nov 17, 2020

jpata commented Nov 17, 2020

fwyzard commented Nov 17, 2020

jpata commented Nov 17, 2020

cmsbuild commented Nov 17, 2020

silviodonato commented Nov 17, 2020

mariadalfonso commented Nov 15, 2020 •

edited

cmsbuild commented Nov 15, 2020 •

edited

cmsbuild commented Nov 16, 2020 •

edited