Skip to content

fix: cuda atomicMin and atomicMax for <double>s#3545

Merged
ianna merged 5 commits intoscikit-hep:mainfrom
Moelf:fix_cuda_atomicMinMax_double
Jun 23, 2025
Merged

fix: cuda atomicMin and atomicMax for <double>s#3545
ianna merged 5 commits intoscikit-hep:mainfrom
Moelf:fix_cuda_atomicMinMax_double

Conversation

@Moelf
Copy link
Copy Markdown
Contributor

@Moelf Moelf commented Jun 14, 2025 •
edited
Loading

fix #3528

the root cause is that the trick for float does not work for double due to, eh, IEEE754 ... My understanding is you can't cast double into long long and still have a total order like you would have for float -> int. See also: https://stackoverflow.com/questions/55140908/can-anybody-help-me-with-atomicmin-function-syntax-for-cuda/55145948#55145948

@ianna ianna changed the title fixing cuda atomicMin and atomicMax for <double>s fix: cuda atomicMin and atomicMax for <double>s Jun 14, 2025
@codecov
Copy link
Copy Markdown

codecov bot commented Jun 14, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.41%. Comparing base (b749e49) to head (548ecd8).
Report is 366 commits behind head on main.

Additional details and impacted files

see 195 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Moelf
Copy link
Copy Markdown
Contributor Author

Moelf commented Jun 14, 2025

related question, how is the CUDA backend tested? If there's CI I would add a test too, but I don't see any... yet

@ikrommyd
Copy link
Copy Markdown
Collaborator

ikrommyd commented Jun 14, 2025

It’s not tested in ci yet. @ariostas has an open PR for that. Typically @ianna runs the tests offline. I could sometimes also do it but I’m not in the US. I don’t have access to my desktop.

@ianna
Copy link
Copy Markdown
Member

ianna commented Jun 14, 2025

related question, how is the CUDA backend tested? If there's CI I would add a test too, but I don't see any... yet

@ariostas has opened a PR for CUDA integration testing

@ariostas
Copy link
Copy Markdown
Member

ariostas commented Jun 17, 2025

The GPU CI is live now!

@ianna
Copy link
Copy Markdown
Member

ianna commented Jun 17, 2025

The GPU CI is live now!

@ariostas - many thanks! I think, we need to add tests-cuda-kernels and tests-cuda-kernels-explicit - they are generated.

@ariostas
Copy link
Copy Markdown
Member

ariostas commented Jun 17, 2025

I think, we need to add tests-cuda-kernels and tests-cuda-kernels-explicit - they are generated.

Oh okay, I missed that part. I assumed that it only generated cpu kernels

ianna
ianna approved these changes Jun 23, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@ianna ianna left a comment

Choose a reason for hiding this comment

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

@Moelf - Looks good! Thanks! When #3551 is merged, please, re-base this one.

@ianna ianna enabled auto-merge (squash) June 23, 2025 18:59
@ianna ianna merged commit f9d8c41 into scikit-hep:main Jun 23, 2025
44 checks passed
@Moelf Moelf deleted the fix_cuda_atomicMinMax_double branch June 23, 2025 19:09
@ianna
Copy link
Copy Markdown
Member

ianna commented Jun 26, 2025

@all-contributors please add @Moelf for code

@allcontributors
Copy link
Copy Markdown
Contributor

allcontributors bot commented Jun 26, 2025

@ianna

I've put up a pull request to add @Moelf! 🎉

@allcontributors allcontributors bot mentioned this pull request Jun 26, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ianna ianna ianna approved these changes

+1 more reviewer

@RongkunWang RongkunWang RongkunWang left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Issues with min() and argmin() with cuda backend

5 participants

@Moelf @ikrommyd @ianna @ariostas @RongkunWang