return 0 in case of invalid detId, fixes cuda sign conversion warning

[smuzaffar]

Just like https://github.com/cms-sw/cmssw/blob/master/RecoParticleFlow/PFRecHitProducer/plugins/alpaka/CalorimeterDefinitions.h#L184-L193 , return 0 (instead of -1) for invalid detId. This should silence the cuda warning [a] which was previously ignore due to bot not using the correct regexp to capture cuda warnings

[a] https://cmssdt.cern.ch/SDT/cgi-bin/buildlogs/el8_amd64_gcc12/CMSSW_14_1_X_2024-06-13-2300/RecoParticleFlow/PFRecHitProducer

>> Compiling alpaka/cuda edm plugin src/RecoParticleFlow/PFRecHitProducer/plugins/alpaka/PFRecHitTopologyESProducer.cc
  src/RecoParticleFlow/PFRecHitProducer/plugins/alpaka/CalorimeterDefinitions.h(108): warning #68-D: integer conversion resulted in a change of sign
         return -1;

[cmsbuild]

cms-bot internal usage

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45223/40591

• This PR adds an extra 16KB to repository

[cmsbuild]

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

It involves the following packages:

• RecoParticleFlow/PFRecHitProducer (reconstruction)

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

cms-bot commands are listed here

[smuzaffar]

please test

[mmusich]

I am confused. Is this undoing #44788 which in turn undid #44608 ?
@fwyzard FYI

[mmusich]

assign heterogeneous

[cmsbuild]

New categories assigned: heterogeneous

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

[smuzaffar]

hold

ah my bad, I did not check the history of the changes.
Should we change it to max value of uint32_t which is what this function is returning ?

[cmsbuild]

Pull request has been put on hold by @smuzaffar
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

[cmsbuild]

+1

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

Comparison Summary

Summary:

• No significant changes to the logs found
• Reco comparison results: 8 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

[jfernan2]

+1

[fwyzard]

-1

[fwyzard]

Personally I think that disregarding (and disabling) this class of warning is the best course.

However, if they are a problem for you, you could change the return value to static_cast<uint32_t>(-1) or std::numeric_limits<uint32_t>::max() or UINT_MAX.

The important thing is, please, do not change the actual values being used in the code only to make the compiler marginally happier, unless you do so consistently everywhere they are used.

[fwyzard]

@mmusich thanks for the heads up.

[fwyzard]

like https://github.com/cms-sw/cmssw/blob/master/RecoParticleFlow/PFRecHitProducer/plugins/alpaka/CalorimeterDefinitions.h#L184-L193 , return 0 (instead of -1) for invalid detId.

I suspect the ECAL code should also be changed to use -1 or UINT_MAX.

That is currently not an issue, because we are not running the ECAL PF reconsruction on GPUs, but it could be a good idea to make the same change for consistency - after double checking that it really doesn't break anything.

[smuzaffar]

closing it in favor of #45238