Skip to content

Conversation

@huangxu96
Copy link
Contributor

@huangxu96 huangxu96 commented Dec 7, 2021

PR types

New features

PR changes

APIs

Describe

Paddle new APIs: put_along_axis

@paddle-bot-old
Copy link

paddle-bot-old bot commented Dec 7, 2021

Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

@huangxu96 huangxu96 changed the title Gather Take_along_axis Dec 7, 2021
@huangxu96 huangxu96 changed the title Take_along_axis Take_along_axis &Put_along_axis Dec 14, 2021
Copy link
Member

@zhhsplendid zhhsplendid left a comment

Choose a reason for hiding this comment

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

Already a good job. However the code is too long so that I left a lot of comments. Can you split the PR in the future? (You don't have to do it in this PR) It seems the PR exceeding 1000 lines would require Dianhai to review (that may be slow ...)

ops::TakeAlongAxisOpKernel<double>,
ops::TakeAlongAxisOpKernel<int>,
ops::TakeAlongAxisOpKernel<uint8_t>,
ops::TakeAlongAxisOpKernel<int64_t>);

Choose a reason for hiding this comment

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

Is it possible to register for complex64 and complex128?

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 don't think so, since the python API level won't support complex type.

Copy link

@iclementine iclementine Dec 30, 2021

Choose a reason for hiding this comment

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

Is complex type not supported in python?
There are some python APIs with complex dtype support.

Copy link

@iclementine iclementine left a comment

Choose a reason for hiding this comment

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

  1. There are some suggesting on code style and sementic issues;
  2. implementation of the gradient of put along aixs seems errorous(both the gradient of input & value should be computed, correctly). I think this is also the reason why it cannot pass grad check.
  3. add docstring for APIs.

@huangxu96 huangxu96 changed the title Take_along_axis &Put_along_axis Put_along_axis Dec 28, 2021
@huangxu96 huangxu96 force-pushed the gather branch 3 times, most recently from e03d420 to 31715a0 Compare December 28, 2021 12:19
@huangxu96 huangxu96 force-pushed the gather branch 2 times, most recently from fc1e7e8 to ee0c213 Compare December 28, 2021 18:49
VLOG(3) << "<<<< Done gpu_scatter_mul_kernel <<<<<";
}

namespace plat = paddle::platform;
Copy link
Member

Choose a reason for hiding this comment

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

It seems you didn't change what I said...

@huangxu96
Copy link
Contributor Author

huangxu96 commented Dec 29, 2021

  1. There are some suggesting on code style and sementic issues;
  2. implementation of the gradient of put along aixs seems errorous(both the gradient of input & value should be computed, correctly). I think this is also the reason why it cannot pass grad check.
  3. add docstring for APIs.
  1. Code style fixed for some parts we share the same idea.
  2. Gradient bug fixed.
  3. Docstring added.

using framework::OperatorWithKernel::OperatorWithKernel;

void InferShape(framework::InferShapeContext* ctx) const override {
PADDLE_ENFORCE_EQ(
Copy link
Contributor

@chenwhql chenwhql Dec 30, 2021

Choose a reason for hiding this comment

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

这里建议使用OP_INOUT_CHECK

zhhsplendid added a commit that referenced this pull request Dec 31, 2021
Paddle new APIs: put_along_axis.

Xu Huang is on holiday so we created this PR to work on it. It is based on his PR: #37921
@huangxu96 huangxu96 closed this Jan 5, 2022
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