-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Not Running the CA When Starting Layers are Empty #49286
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
base: master
Are you sure you want to change the base?
Conversation
|
cms-bot internal usage |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49286/46648
|
|
A new Pull Request was created by @AdrianoDee for master. It involves the following packages:
@cmsbuild, @jfernan2, @mandrenguyen, @srimanob can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
enable gpu |
|
please test |
|
+1 Size: This PR adds an extra 36KB to repository Comparison SummarySummary:
AMD_MI300X Comparison SummaryThere are some workflows for which there are errors in the baseline: Summary:
AMD_W7900 Comparison SummarySummary:
NVIDIA_H100 Comparison SummaryThere are some workflows for which there are errors in the baseline: Summary:
NVIDIA_L40S Comparison SummaryThere are some workflows for which there are errors in the baseline: Summary:
NVIDIA_T4 Comparison SummaryThere are some workflows for which there are errors in the baseline: Summary:
|
|
+1 |
|
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. @mandrenguyen, @sextonkennedy, @ftenchini (and backports should be raised in the release meeting by the corresponding L2) |
|
@AdrianoDee The merge with #49282 gave a conflict, as expected. If you can fix it quickly, I can still get it in for the 1100IB |
|
@mandrenguyen I need to act on the PR so we can postpone the integration for the moment. |
| auto& queue = iEvent.queue(); | ||
| const auto device = alpaka::getDev(queue); | ||
| reco::TracksSoACollection tracks({{0, 0}}, queue); | ||
| auto ntracks_d = cms::alpakatools::make_device_view(device, tracks.view().nTracks()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should be able to pass directly the queue:
| auto ntracks_d = cms::alpakatools::make_device_view(device, tracks.view().nTracks()); | |
| auto ntracks_d = cms::alpakatools::make_device_view(queue, tracks.view().nTracks()); |
@AdrianoDee You mean a non-trivial update beyond the merge conflict? If it's acceptable we wil first build a patch and then come back for this in a subsequent full release sometime over the next week. I suppose this is not so crucial for HI data taking as we're not running much tracking at HLT. |
|
@mandrenguyen Yes, it's fine, it not crucial. |
PR description:
This PR proposes a small improvement for the CA seeding (inspired by the debugging of #49186, in which many events have no hits on BPix1). As is, when BPix1 is empty, we run the CA anyway, even if all the starting pairs have BPix1 as the inner layer. This is useless and not easy to spot. Instead of running the CA, now a
LogWarningis printed. In principle, it would make sense to generalize this to all the possible starting layers (I wouldn't push this in this PR since it would need non-trivial changes).Note: since I was working on #49186, I've included 49b8b8a from #49282 here.