Skip to content

Conversation

@kakwok
Copy link
Contributor

@kakwok kakwok commented Jun 11, 2024

PR description:

A subset of changes from #44910, including only the data format changes for HcalRecHit SoA output.
Since the output data format is stable, this PR allows @jsamudio in parallel to develop for the changes on the PF cluster side to use HcalRecHit SoA as input.

PR validation:

See #44910.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 11, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45199/40549

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • DataFormats/HcalRecHit (reconstruction)

@jfernan2, @cmsbuild, @mandrenguyen can you please review it and eventually sign? Thanks.
@missirol, @bsunanda, @rovere, @mariadalfonso, @abdoulline this is something you requested to watch as well.
@sextonkennedy, @rappoccio, @antoniovilela you are the release manager for this.

cms-bot commands are listed here

@mandrenguyen
Copy link
Contributor

@kakwok is there a relval workflows that tests this PR, or not yet?

@kakwok
Copy link
Contributor Author

kakwok commented Jun 17, 2024

@mandrenguyen Not yet, the workflow for testing will need #44910 .

@fwyzard
Copy link
Contributor

fwyzard commented Jun 17, 2024

assign heterogeneous

@cmsbuild
Copy link
Contributor

New categories assigned: heterogeneous

@fwyzard,@makortel you have been requested to review this Pull request/Issue and eventually sign? Thanks


GENERATE_SOA_LAYOUT(HcalRecHitSoALayout,
SOA_SCALAR(uint32_t, size),
SOA_COLUMN(uint32_t, did),
Copy link
Contributor

Choose a reason for hiding this comment

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

could you change did to detId ?

<use name="DataFormats/DetId"/>
<use name="DataFormats/Portable"/>
<use name="DataFormats/SoATemplate"/>
<use name="FWCore/MessageLogger"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<use name="FWCore/MessageLogger"/>

This doesn't seem to be needed.

<use name="DataFormats/Common"/>
<use name="DataFormats/HcalDetId"/>
<use name="DataFormats/HcalDigi"/>
<use name="DataFormats/DetId"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<use name="DataFormats/DetId"/>

This one doesn't seem to be needed, either.

@fwyzard
Copy link
Contributor

fwyzard commented Jun 17, 2024

In addition, @jsamudio is working on updating the alpaka PF clustering code to replace the ad hoc SoA they are currently using with this one.

@mandrenguyen
Copy link
Contributor

mandrenguyen commented Jun 17, 2024

@mandrenguyen Not yet, the workflow for testing will need #44910 .

Thanks, I was just trying to understand the expectation. So I guess the proposal is just to visually inspect this and integrate for upcoming developments. Looks straightforward anyway.

@fwyzard
Copy link
Contributor

fwyzard commented Jun 17, 2024

@mandrenguyen of course. To clarify: the idea is to merge the data formats from #44910 early, while the validation of the algorithms is still ongoing, to let the PF code adopt them.

@fwyzard
Copy link
Contributor

fwyzard commented Jun 17, 2024

allow @kakwok test rights

@fwyzard
Copy link
Contributor

fwyzard commented Jun 17, 2024

@kakwok could you address the small comments I left, and then ask "please test" so the bot can verify the changes ?

@kakwok
Copy link
Contributor Author

kakwok commented Jun 17, 2024

@cmsbuild please test

@cmsbuild
Copy link
Contributor

Pull request #45199 was updated. @jfernan2, @mandrenguyen can you please check and sign again.

@fwyzard
Copy link
Contributor

fwyzard commented Jun 18, 2024

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-55664a/39932/summary.html
COMMIT: c73ad34
CMSSW: CMSSW_14_1_X_2024-06-17-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/45199/39932/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially removed 1 lines from the logs
  • Reco comparison results: 12 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3345018
  • DQMHistoTests: Total failures: 6
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3344992
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 202 log files, 165 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@mandrenguyen
Copy link
Contributor

@fwyzard
Copy link
Contributor

fwyzard commented Jun 18, 2024

@mandrenguyen thanks for looking into this.

The bot is noting a coding rule violation here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-55664a/39932/codeRules/cmsCodeRule1.txt

Let's call this a false positive.

The bot should warn about using namespace in the global namespace.

What the code is doing here is using namespace hcal inside the ALPAKA_ACCELERATOR_NAMESPACE namespace, which is not an issue.

and a duplicate dictionary warning here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-55664a/39932/dupDict/

This is a real issue, I will look into it.

@fwyzard
Copy link
Contributor

fwyzard commented Jun 19, 2024

and a duplicate dictionary warning here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-55664a/39932/dupDict/

This is a real issue, I will look into it.

Actually after going through CMSSW I cannot find anything called hcal::HcalRecHitSoA or even HcalRecHitSoA.

The CUDA code uses hcal::RecHitCollection<...>, so there are no conflicts there.

@fwyzard
Copy link
Contributor

fwyzard commented Jun 19, 2024

@mandrenguyen I think we can go ahead and merge this.

@fwyzard
Copy link
Contributor

fwyzard commented Jun 19, 2024

@kakwok could you make a PR with a backport to 14.0.9 ?

@fwyzard
Copy link
Contributor

fwyzard commented Jun 20, 2024

@kakwok could you make a PR with a backport to 14.0.9 ?

Done in #45277 .

@mandrenguyen
Copy link
Contributor

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

@mandrenguyen
Copy link
Contributor

type hcal

@cmsbuild cmsbuild added the hcal label Jun 20, 2024
@antoniovilela
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.

5 participants