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

Adding support for the sigmoid_cross_entropy_with_logits operator #4448

Conversation

abhinavarora
Copy link
Contributor

@abhinavarora abhinavarora commented Sep 27, 2017

This PR closes #4318

@abhinavarora abhinavarora self-assigned this Sep 27, 2017
namespace paddle {
namespace operators {

using Tensor = framework::Tensor;
Copy link
Contributor

Choose a reason for hiding this comment

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

use using in the header file violate the google style, see the detail in Namespace

Do not use Namespace aliases at namespace scope in header files except in explicitly marked internal-only namespaces, because anything imported into a namespace in a header file becomes part of the public API exported by that file.

By the way, I think

using framework::Tensor;

is enough here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pointing this out. I will remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. Fixed in ab15457

"(Tensor, default Tensor<float>), a 2-D tensor of the same type "
"and shape as X. This input is a tensor of probabalistic labels "
"for each logit");
AddOutput("Y",
Copy link
Contributor

Choose a reason for hiding this comment

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

single output name prefers to Out, to be unified with other operators.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the suggestion. Fixed in ab15457

@reyoung reyoung requested a review from QiJune September 27, 2017 22:57
sigmoid_X = expit(self.inputs['X'])
term1 = self.inputs['Labels'] * np.log(sigmoid_X)
term2 = (1 - self.inputs['Labels']) * np.log(1 - sigmoid_X)
self.outputs = {'Y': -term1 - term2}
Copy link
Member

Choose a reason for hiding this comment

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

-term1 - term2

to

- term1 - term2

add a space

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the comment. Fixed in ab15457 .

}
};

// TODO(aroraabhinav) : Complete proto documentation
Copy link
Member

Choose a reason for hiding this comment

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

Is this TODO still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pointing this out. I will remove this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in ab15457 .

Copy link
Contributor

@dzhwinter dzhwinter left a comment

Choose a reason for hiding this comment

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

LGTM!
by the way, we'd better implement the new operator through the fundamental math operators. That's will save a lot of test code and easier for optimize. :)

@abhinavarora abhinavarora merged commit b9336e6 into PaddlePaddle:develop Sep 28, 2017
@abhinavarora abhinavarora deleted the sigmoid_cross_entropy_with_logits branch September 28, 2017 18:02
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.

sigmoid_cross_entropy_with_logits_op
4 participants