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.

Sign up for GitHub

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

Jump to bottom

Inherit input ndarray device in chainerx.ascontiguousarray #8262

Merged
merged 14 commits into from Oct 17, 2019
Merged

Inherit input ndarray device in chainerx.ascontiguousarray #8262

merged 14 commits into from Oct 17, 2019

Conversation

take-cheeze
Copy link
Member

When is device=None.
closes #8259

@niboshi
Copy link
Member

niboshi commented Oct 9, 2019

How about adding device checks to existing tests, instead of creating a new test?

@take-cheeze
Copy link
Member Author

I want to parametrize cuda too but there is no grantee of two GPUs on the system so specialized it.

@niboshi
Copy link
Member

niboshi commented Oct 9, 2019

@pytest.mark.parametrize_device can automatically skip devices on systems without that device.

@take-cheeze
Copy link
Member Author

Seems @pytest.mark.parametrize_device doesn't support targeting multiple device and @pytest.mark.cuda(x) can't handle native so seems testing native is enough for now.

@niboshi
Copy link
Member

niboshi commented Oct 9, 2019

targeting multiple device

Do you mean the default device?
I think it would be better to follow my previous suggestion if @pytest.mark.parametrize_device did not change the default device, but I got the point.

@niboshi niboshi self-assigned this Oct 9, 2019
@niboshi niboshi added the cat:bug Bug report or fix. label Oct 9, 2019
@niboshi niboshi added this to the v7.0.0rc1 milestone Oct 9, 2019
@take-cheeze take-cheeze added the ChainerX Related to ChainerX. label Oct 9, 2019
@take-cheeze
Copy link
Member Author

take-cheeze commented Oct 10, 2019 •
edited

As discussed for #8273 I've removed the comment

@niboshi
Copy link
Member

niboshi commented Oct 10, 2019

Aren't we removing device argument?

@take-cheeze
Copy link
Member Author

@niboshi I'd like to do the interface change in separate PR with to_device stuff for #8273 .
This PR is only for bug fix of #8259

@niboshi
Copy link
Member

niboshi commented Oct 10, 2019

@take-cheeze
I see, thanks.
I think it's better to remove device argument first, because a fix for this PR would be much simpler.
But I found device argument was introduced for a reason: because chainerx.ascontiguousarray (not chainerx::AsContiguousArray) can accept numpy.ndarray.
We need to discuss whether or not this spec is necessary first.

@niboshi
Copy link
Member

niboshi commented Oct 11, 2019

As it's safer to start with a minimal specification, let's remove numpy.ndarray support and device argument from ascontiguousarray.

@niboshi
Copy link
Member

niboshi commented Oct 11, 2019

Note that the argument a of ascontiguousarray can be const ArrayBodyPtr&.
The device can be directly retrieve from it and the fix to MakeArray will be unnecessary.

@take-cheeze take-cheeze mentioned this pull request Oct 15, 2019
Merged
1 task
niboshi
niboshi requested changes Oct 15, 2019
View reviewed changes
tests/chainerx_tests/unit_tests/routines_tests/test_creation.py Outdated
@@ -383,12 +348,23 @@ def test_ascontiguousarray_from_chainerx_array(device, shape, dtype, padding):
a = chainerx.ascontiguousarray(obj)
if not padding and shape != (): # () will be reshaped to (1,)
assert a is obj
e = chainerx.ascontiguousarray(np_arr)
e = chainerx.asarray(np_arr)
Copy link
Member

Choose a reason for hiding this comment

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

np_arr should just work in chainerx.testing.assert_array_equal_ex. How about simply removing this line?

Copy link
Member Author

Choose a reason for hiding this comment

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

I like the variable name so removed the function calling instead!

@niboshi
Copy link
Member

niboshi commented Oct 17, 2019

Jenkins, test this please

@niboshi niboshi added the st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes. label Oct 17, 2019
@chainer-ci
Copy link
Member

Jenkins CI test (for commit 3d1d5ef, target branch master) succeeded!

@mergify mergify bot merged commit a6a4026 into chainer:master Oct 17, 2019
@take-cheeze take-cheeze deleted the fix_8259 branch October 17, 2019 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@niboshi niboshi niboshi approved these changes

Assignees

@niboshi niboshi

Labels
cat:bug Bug report or fix. ChainerX Related to ChainerX. st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes.
Projects
None yet
Milestone
v7.0.0rc1
Development

Successfully merging this pull request may close these issues.

chainerx.ascontiguousarray() does not preserve the backend of the array
3 participants
@take-cheeze @niboshi @chainer-ci