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.

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

Update reducer types allowed in parallel reduction. #221

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pkestene
Copy link
Contributor

@pkestene pkestene commented Dec 1, 2023

Trying to fix #220

Copy link
Contributor

@NaderAlAwar NaderAlAwar left a comment

Choose a reason for hiding this comment

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

The changes look good. Would it be possible to add a test that confirms that this is working fine? I think you can extend this test

def test_squaresum_types(series_max, dtype):

by simply parameterizing it on the other data types you added.

@pkestene
Copy link
Contributor Author

pkestene commented Dec 1, 2023

The changes look good. Would it be possible to add a test that confirms that this is working fine? I think you can extend this test

def test_squaresum_types(series_max, dtype):

by simply parameterizing it on the other data types you added.

Sure, I'll update the test.

But just to clarify beforehand:
in

def squaresum(self, i: float, acc: pk.Acc[pk.double]):
the iterate variable is a float, but should really be an integral type. I was even surprised the test builds. I think, kokkos should check that internally, no ?

@NaderAlAwar
Copy link
Contributor

the iterate variable is a float, but should really be an integral type. I was even surprised the test builds. I think, kokkos should check that internally, no ?

Looking at the generated C++, it seems that PyKokkos ignores the type supplied by the user and generates an int32_t. In Kokkos, it is possible to set the index type through a RangePolicy template argument, but we do not support that yet. I think the test should be changed to an int iterate variable for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Example random_sum.py fails because int32 is not a supported type for a reduction
2 participants