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

Use Variable.array instead of Variable.data #336

Merged
merged 6 commits into from
Oct 17, 2018

Conversation

muupan
Copy link
Member

@muupan muupan commented Oct 17, 2018

Resolves #335

Chainer v2 support is dropped.

Both Variable.shape and Variable.dtype are ready since Chainer v2, so
they are used to shrink lines that exceeds 80.
pytest was introduced at v3.1.0.
@muupan muupan requested a review from toslunar October 17, 2018 13:32
@toslunar toslunar self-assigned this Oct 17, 2018
@toslunar
Copy link
Member

Once we drop Chainer v2 support, we can always use F.matmul in place of matmul_v3. This can be done in another PR.

@muupan
Copy link
Member Author

muupan commented Oct 17, 2018

It seems pytest-cov was missing for v3.1.0 CI, so I added.

@@ -17,15 +17,15 @@ class PrioritizedBuffer (object):
def __init__(self, capacity=None, wait_priority_after_sampling=True,
initial_max_priority=1.0):
self.capacity = capacity
self.data = collections.deque()
self.array = collections.deque()
Copy link
Member

Choose a reason for hiding this comment

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

.data in the file is not an attribute of Variable.

@@ -21,7 +21,7 @@ def backward(self, inputs, grads):

def forward_gpu(self, inputs):
n = len(inputs)
ptrs = cuda.cupy.asarray([x.data.ptr for x in inputs],
ptrs = cuda.cupy.asarray([x.array.ptr for x in inputs],
Copy link
Member

Choose a reason for hiding this comment

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

Please revert the changes to the file.

Copy link
Member

@toslunar toslunar left a comment

Choose a reason for hiding this comment

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

LGTM. I confirmed that all .data in **/*.py is not the attribute of Variable.
I'll merge the PR after I run gpu/slow tests.

@muupan
Copy link
Member Author

muupan commented Oct 17, 2018

Thanks for your great review!

@toslunar toslunar merged commit c85a20c into chainer:master Oct 17, 2018
@toslunar toslunar added this to the v0.5 milestone Oct 18, 2018
@muupan muupan modified the milestone: v0.5 Nov 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants