-
Notifications
You must be signed in to change notification settings - Fork 66
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
fix to allow for converting to Int #273
Conversation
I didn't mention it, but here is my test script:
I had it wrapped in a test set when testing shared memory, but now just have it wrapped in The error is
|
As another important note, changing the InexactError to a print ( For some reason only works on 2 threads before saying that we are multiplying 2 print statements. I think the error above is because Here is the error for when we swap it out with a print:
Since the task is a nested task error, I did try this on the latest Julia version from github to make sure that it wasn't somehow a catch with this issue: #232 |
bors try |
tryBuild failed: |
I pushed 2 commits here. I tried a bunch of different ways of iterating through the types, such as the one shown in the previous commit, but also just sending a bunch of types in via CuArray and using tid in parallel. Honestly, none of them worked on the GPU, so I just manually unrolled everything. I don't think it's worth messing with this code to make it that pretty, but if you want to try to get the loops to work, feel free. Also: should we have excluded this from ROC? I have not been able to test this on AMD, but I would guess it would work as well? |
test/convert.jl
Outdated
@@ -0,0 +1,35 @@ | |||
using CUDA, CUDAKernels |
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.
Shouldn't need these using lines as part of the test-suite
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.
yup, I will fix that. Are you ok with the unrolled test? If so, I'll squash now as well.
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.
The test are a bit odd since you will always convert to eltype(B)
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.
Yeah, I mean, the outpur doesn't really matter too much. We don't have to output anything, but can instead just run Float64() on all of them.
Then there wouldn't be an @test
, though.
I'll rewrite everything so each row in B will be another one of the unrolled tests. Give me a few minutes...
I re-wrote the tests so that B now has a different row for each integer conversion. We could have a bunch of other arrays for different floating types, but those don't have the inexact errors, so converting to any floating type is fine. My broadcasting magic is a bit weak here. I was able to do 3 tests on the GPU without the for loop, but it did not work on the CPU, so we now have 30 tests. |
By the way, I am happy to squash the commits here if you are happy with the new test |
LGTM |
Ok, squashed |
A bit lost with this PR, but it is an attempt at a fix for #254 and #265.
I posted the error messages on those Issues and have a few lines here that I've played around with. I am ultimately trying to replace the InexactError with a working version