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

Fix Transform API for operator #4209

Closed

Conversation

reyoung
Copy link
Collaborator

@reyoung reyoung commented Sep 19, 2017

@reyoung reyoung requested a review from qingqing01 September 19, 2017 22:40
@@ -31,13 +31,14 @@ namespace paddle {
namespace platform {
// Transform on host or device. It provides the same API in std library.
template <typename InputIter, typename OutputIter, typename UnaryOperation>
void Transform(const DeviceContext& context, InputIter first, InputIter last,
OutputIter result, UnaryOperation op) {
inline static void Transform(const DeviceContext& context, InputIter first,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since we use static, the Transform will not generate symbols. cc or cu will use different Transform method.

Copy link
Contributor

@typhoonzero typhoonzero Sep 20, 2017

Choose a reason for hiding this comment

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

Thought it was the inline keyword that makes Transform not generating symbols?

Markup: https://stackoverflow.com/questions/780730/c-c-static-function-in-header-file-what-does-it-mean

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think this solution is better than #4201. There are two problems with this solution:

  1. The Transform interface in .cu.o has a useless cpu code branch.
  2. If there are many .cc and .cu are used Transform interface, will compile a lot of duplicate code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe we should force inline? I think using a functor is a little bit complex.

Copy link
Collaborator Author

@reyoung reyoung Sep 20, 2017

Choose a reason for hiding this comment

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

  1. We may invoke CPU Transform in .cu file, so that branch is not useless. Also, if we make place as a template parameter, we should use PADDLE_ENFORCE to check the template place is same as the context.GetPlace(). So the if branch will not be removed no matter how we implement Transform.

  2. In fact, the BinaryFunctor, UnaryFunctor and the Iterator are the template parameter. Each time we invoke Transform, a new code will be generated. No matter how we implement it.

To make Transform a simple method, not a templated functor, can make users invoke it easily.

It is easier for user write Transform(ctx, input, input+size, output, Functor); than Transform<Place>()(ctx, input, input+size, output, Functor);.

@@ -17,7 +17,7 @@ def setUp(self):
assert out_np is not self.inputs['X']
self.outputs = {'Out': out_np}

def not_test_check_output(self):
def test_check_output(self):
self.check_output()

def not_test_check_grad(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

not_test_check_grad -> test_check_grad

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@reyoung reyoung closed this Sep 20, 2017
@reyoung reyoung deleted the feature/transform_api_for_op branch October 28, 2017 22:16
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.

4 participants