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

Make elementwise_op supporting scalar input Y #7593

Merged
merged 16 commits into from
Jan 18, 2018

Conversation

JiayiFeng
Copy link
Collaborator

@JiayiFeng JiayiFeng commented Jan 17, 2018

solve #7568

// y is a scalar
auto extended_dims = framework::vectorize(x_dims);
extended_dims.push_back(1);
x_dims = framework::make_ddim(extended_dims);
Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, axis should be x->dims().size(), otherwise, it is wrong.
So I think this section of the code should be like this:

  int axis = ctx.Attr<int>("axis");
  axis = (axis == -1 ? x_dims.size() - y_dims.size() : axis);

  if (y_dims.size() == 1 && y_dims[0] == 1) {
    // y is a scalar
   axis = x_dims.size();
    auto extended_dims = framework::vectorize(x_dims);
    extended_dims.push_back(1);
    x_dims = framework::make_ddim(extended_dims);
  }

 

Copy link
Collaborator Author

@JiayiFeng JiayiFeng Jan 17, 2018

Choose a reason for hiding this comment

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

I don't quite agree with that. If the axis is inconsistent with the actual data shape, the code shall give an error instead of modifying axis in silence. Because we can never know which one is wrong, the axis or the data shape.

Copy link
Contributor

@chengduoZH chengduoZH left a comment

Choose a reason for hiding this comment

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

LGTM

@JiayiFeng JiayiFeng merged commit 1d89866 into PaddlePaddle:develop Jan 18, 2018
@JiayiFeng JiayiFeng deleted the dev_elementwise_scalar branch January 18, 2018 02:02
@gongweibao
Copy link
Contributor

gongweibao commented Jan 18, 2018

This PR is large. I think it's better to split elementwise_min_op elementwise_max_op elementwise with the support that Y is scalar into separate PRs.

Thanks @JiayiFeng, for the support that argument Y can be a scalar.

@JiayiFeng
Copy link
Collaborator Author

JiayiFeng commented Jan 18, 2018

@gongweibao
I did. The PR of elementwise_max/min is #7538 .
This one depends on 7538. However, I can't merge 7538 yesterday because of the CI trouble. To avoid blocking my job, I merged 7538 to this PR on my own computer. So now this PR contains 7538.

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.

3 participants