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

Feature/copytensor #5455

Merged
merged 19 commits into from
Nov 26, 2017
Merged

Conversation

dzhwinter
Copy link
Contributor

@dzhwinter dzhwinter commented Nov 8, 2017

fix #4647

This PR contains

  1. fix Remove CopyFrom and CopyFromVector from Tensor interface and make them global functions #4647 , move utility function out of Tensor.
  2. add a CopyToVector Interface.
  3. Delete unused tensorarray, dynamic_recurrent_rnn. We have another implementation. @Superjom
  4. fix the gru_unit_op in debug mode.

@dzhwinter dzhwinter mentioned this pull request Nov 8, 2017
@dzhwinter dzhwinter mentioned this pull request Nov 16, 2017
3 tasks
Copy link
Contributor

@typhoonzero typhoonzero left a comment

Choose a reason for hiding this comment

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

Putting non-member functions to a separated place is awesome.

@@ -5,8 +5,13 @@ cc_library(ddim SRCS ddim.cc DEPS eigen3)
cc_test(ddim_test SRCS ddim_test.cc DEPS ddim)
nv_test(dim_test SRCS dim_test.cu DEPS ddim)

# cc_library(tensor SRCS tensor.cc tensor_util.cc DEPS ddim place paddle_memory device_context)
Copy link
Contributor

Choose a reason for hiding this comment

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

No commented out codes please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Superjomn
Superjomn previously approved these changes Nov 23, 2017
Copy link
Contributor

@Superjomn Superjomn left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@typhoonzero typhoonzero left a comment

Choose a reason for hiding this comment

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

LGTM++

@@ -0,0 +1,36 @@
import unittest
Copy link
Contributor

Choose a reason for hiding this comment

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

should remove this file since or put under v2/fluid/test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have another PR in of #5454. which I can move this file to the correct place in that PR.
This PR is so huge and fragile, I think we can mark that and do it later.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK

Copy link
Contributor

@typhoonzero typhoonzero left a comment

Choose a reason for hiding this comment

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

LGTM++

@@ -0,0 +1,36 @@
import unittest
Copy link
Contributor

Choose a reason for hiding this comment

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

OK

@dzhwinter dzhwinter merged commit 45062fe into PaddlePaddle:develop Nov 26, 2017
@jacquesqiao
Copy link
Member

please clear code before merge if code is too old

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.

Remove CopyFrom and CopyFromVector from Tensor interface and make them global functions
4 participants