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

Add test for reverse slicing #162

Closed
wants to merge 7 commits into from
Closed

Add test for reverse slicing #162

wants to merge 7 commits into from

Conversation

loikki
Copy link
Contributor

@loikki loikki commented Nov 20, 2017

Hi,

I am currently trying to use reverse slicing with a gpuarray (e.g. [end:start:-step]), but I am getting an error. I have written quickly a new test in order to reproduce the bug (feel free to discard my merge request if you are able to work on it).

>       copy.src_pitch = src_strides[1]
E       OverflowError: can't convert negative value to unsigned int

/usr/local/lib/python3.5/dist-packages/pycuda-2017.1.1-py3.5-linux-x86_64.egg/pycuda/gpuarray.py:1300: OverflowError

I have been through the code looking for the definition of src_pitch and I suppose that it comes from cudaMemcpy defined by Nvidia, right? Therefore, it would not be possible to change the unsigned int to a signed one.

@inducer
Copy link
Owner

inducer commented Nov 20, 2017

a_gpu_slice = a_gpu[end:start:-2]
a_slice = a[start:end]

assert la.norm(a_gpu_slice.get()-a_slice) == 0

Those are some weird semantics. I'd call that a bug. I'd prefer this to be true:

a_gpu_slice = a_gpu[end:start:-2]
a_slice = a[end:start:-2]

assert la.norm(a_gpu_slice.get()-a_slice) == 0

@loikki
Copy link
Contributor Author

loikki commented Nov 21, 2017

Sorry, I was a little bit too fast in writing the test.

The error was not on the assert, but on the slicing a_gpu_slice = a_gpu[end:start:-2]

I have just fixed the test

@inducer
Copy link
Owner

inducer commented Nov 21, 2017

That looks OK. I'd be happy to take a patch that makes this pass.

@loikki
Copy link
Contributor Author

loikki commented Nov 22, 2017

I suppose that we need to trick cuda in order avoid the negative value in an unsigned int.
What do you think about selecting in the normal direction and then reversing it?

@inducer
Copy link
Owner

inducer commented Nov 22, 2017

I'd be OK with that, as long as it gives rise to correct behavior in all cases.

@loikki
Copy link
Contributor Author

loikki commented Nov 24, 2017

Hi,

I have been able to implement it.
To do it, I add to modify the reverse kernel and modify getitem in order to switch start and stop

@inducer
Copy link
Owner

inducer commented Nov 24, 2017

@inducer
Copy link
Owner

inducer commented Nov 24, 2017

I actually don't see why a call to reverse should be necessary. (And I also view that as a negative since it adds a cost that a user might not expect.) I.e. why not copy the array in the "forward" direction and then adjust the strides? Could you explain?

@loikki
Copy link
Contributor Author

loikki commented Nov 24, 2017

I am not sure if it is possible. When I tried to use a negative step, the CUDA api was raising an error due to the use of unsigned int.

Anyway, I think that my work on the reverse function will be useful as now it is able to deal with discontinuous data

@inducer
Copy link
Owner

inducer commented Nov 24, 2017

I am not sure if it is possible. When I tried to use a negative step, the CUDA api was raising an error due to the use of unsigned int.

I think it should be possible. Any data "footprint" that can be reached with negative strides can also be reached with "positive" strides. So, suppose we call that operation an "axis flip". Then you would pick out all the axes with negative strides, flip them, do the copy, and then flip them again. No additional code execution/second-stage data copying needed.

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.

2 participants