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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

test_numba_integration passes tuple into cuda instead of array #70611

Closed
code-review-doctor opened this issue Jan 4, 2022 · 1 comment
Closed
Labels
module: tests Issues related to tests (not the torch.testing module) triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Comments

@code-review-doctor
Copy link
Contributor

code-review-doctor commented Jan 4, 2022

馃悰 Describe the bug

There is an (as far as I can tell) unwanted comma on line 314:

for dtype in dtypes:
numpy_ary = numpy.arange(6).reshape(2, 3).astype(dtype),
numba_ary = numba.cuda.to_device(numpy_ary)
self.assertTrue(numba_ary.is_c_contiguous())
torch_ary = torch.as_tensor(numba_ary, device="cuda")
self.assertTrue(torch_ary.is_contiguous())

This makes the numpy_ary a tuple containing a numpy array rather than a numpy array ie.:

In [3]: numpy.arange(6).reshape(2, 3).astype(numpy.float64)  # no comma at end
Out[3]: 
array([[0., 1., 2.],
       [3., 4., 5.]])

In [4]: numpy.arange(6).reshape(2, 3).astype(numpy.float64),  # comma at end
Out[4]: 
(array([[0., 1., 2.],
        [3., 4., 5.]]),)

This is a problem for the test because the tuple is then passed into numba.cuda.to_device(numpy_ary) and assertions are done on the output. It is possible the test is affected by the fact a tuple is passed in instead of a numpy array. Indeed, if in "live" a numpy array is passed in but in tests a tuple containing a numpy array is passed in then this reduces the confidence the tests give us as it's not "like live".

Versions

NA

I can make a PR to fix this :)

cc @mruberry

@VitalyFedyunin VitalyFedyunin added module: tests Issues related to tests (not the torch.testing module) triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module labels Jan 5, 2022
@mruberry
Copy link
Collaborator

Hi @code-review-doctor, I agree that comma seems out of place and it's odd that the test passes. We would accept a PR removing it (if that really is the intention of the test).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: tests Issues related to tests (not the torch.testing module) triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants