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.

Sign up for GitHub

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

Jump to bottom

PFRecHitTopologyESProducer may access and invalid SoA index #44789

Closed
fwyzard opened this issue Apr 21, 2024 · 9 comments · Fixed by #44790
Closed

PFRecHitTopologyESProducer may access and invalid SoA index #44789

fwyzard opened this issue Apr 21, 2024 · 9 comments · Fixed by #44790
Labels
heterogeneous-pending pending-signatures pf

Comments

@fwyzard
Copy link
Contributor

fwyzard commented Apr 21, 2024

Enabling range checks shows that PFRecHitTopologyESProducer may access and invalid SoA index:

----- Begin Fatal Exception 21-Apr-2024 12:07:14 CEST-----------------------
An exception of category 'StdException' occurred while
   [0] Processing  Event run: 379617 lumi: 3 event: 467845 stream: 0
   [1] Running path 'DST_PFScouting_SingleMuon_v2'
   [2] Prefetching for module PFClusterSoAProducer@alpaka/'hltParticleFlowClusterHBHESoA'
   [3] Prefetching for EventSetup module PFRecHitHCALTopologyESProducer@alpaka/'hltESPPFRecHitHCALTopology'
   [4] Calling method for EventSetup module PFRecHitHCALTopologyESProducer@alpaka/'hltESPPFRecHitHCALTopology'
Exception Message:
A std::exception was thrown.
Out of range index in mutable neighbours(size_type index): index -1 vs size 23328
----- End Fatal Exception -------------------------------------------------
@fwyzard
Copy link
Contributor Author

fwyzard commented Apr 21, 2024

type pf

@fwyzard
Copy link
Contributor Author

fwyzard commented Apr 21, 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

@cmsbuild
Copy link
Contributor

cms-bot internal usage

@cmsbuild
Copy link
Contributor

A new Issue was created by @fwyzard.

@Dr15Jones, @rappoccio, @sextonkennedy, @antoniovilela, @smuzaffar, @makortel can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

This was referenced Apr 21, 2024
Merged
Merged
@fwyzard
Copy link
Contributor Author

fwyzard commented Apr 21, 2024

A minimal fix is available in #44790 (for 14.1.x) and #44791 (for 14.0.x).

@fwyzard
Copy link
Contributor Author

fwyzard commented Apr 21, 2024

@jsamudio @fllor @hatakeyamak while looking into this, I noticed a realised there is a minor inconsistency in the code:

  • neighbours is a matrix of int32_t (signed)
  • detId2denseId returns an uint32_t (unsigned)

Is it intended ?
If it is intended, could you add a static_cast in the appropriate places to mark it as such ?
If it is not intended, could you look for a way to make it uniform ?

In either case, could you introduce a named constant to identify invalid dense ids, instead of using -1 or 0xffffffff ?

Thank you !

@fwyzard fwyzard mentioned this issue Apr 21, 2024
Merged
@fllor
Copy link
Contributor

fllor commented Apr 21, 2024 •
edited

You are right. A denseId should always be a uint_32t with 0xffffffff representing an invalid entry. The named constant also makes sense.

@jsamudio @hatakeyamak Since I do not have access to the CMS infrastructure anymore, could one of you do the commit?
For the int32_t, I think you just need to change these two lines:

cmssw/DataFormats/ParticleFlowReco/interface/PFRecHitSoA.h

Line 14 in 3fb8b0e

using PFRecHitsNeighbours = Eigen::Matrix<int32_t, 8, 1>;

cmssw/RecoParticleFlow/PFRecHitProducer/plugins/alpaka/PFRecHitProducerKernel.dev.cc

Line 158 in 3fb8b0e

pfRecHits.neighbours(i)(n) = (int32_t)pfRecHit_neighbour;

For the named constant, just do grep -rw 0xffffffff RecoParticleFlow/PFRecHitProducer/ to find all occurrences. Though I am not quite sure right now what is happening in this line:

cmssw/RecoParticleFlow/PFRecHitProducer/plugins/alpaka/PFRecHitProducerKernel.dev.cc

Line 33 in 3fb8b0e

const uint32_t j = alpaka::atomicInc(acc, num_pfRecHits, 0xffffffff, alpaka::hierarchy::Blocks{});

This should probably be left unchanged.

Also introduce the constant here:

cmssw/RecoParticleFlow/PFRecHitProducer/plugins/alpaka/CalorimeterDefinitions.h

Line 104 in 3fb8b0e

return 0;

cmssw/RecoParticleFlow/PFRecHitProducer/plugins/alpaka/CalorimeterDefinitions.h

Line 188 in 3fb8b0e

return 0;

This will then also solve #44608

@jsamudio
Copy link
Contributor

I am currently traveling, but will tackle this once I reach my destination.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
heterogeneous-pending pending-signatures pf
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Skip invalid HCAL PF rechits in PFRecHitTopologyESProducer fwyzard/cmssw
4 participants
@fwyzard @cmsbuild @fllor @jsamudio