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

Add nce op #5480

Merged
merged 10 commits into from
Dec 1, 2017
Merged

Add nce op #5480

merged 10 commits into from
Dec 1, 2017

Conversation

wanghaoshuang
Copy link
Contributor

@wanghaoshuang wanghaoshuang commented Nov 8, 2017

Fix #5479

  • Add forward and backward kernel
  • Add unitest
  • Add comment and doc

@zhouxiao-coder
Copy link
Contributor

I think TensorFlow has a nice design about candidate sampling methods and we can use it as a reference. It supports not only NCE, but also importance sampling and negative sampling, which are also popular methods. It also naturally adapts to GPU version.

@lcy-seso
Copy link
Contributor

lcy-seso commented Nov 16, 2017

@zhouxiao-coder We have already discussed with @wanghaoshuang about systematically support the sampling methods. It will be refined later. In this PR, we only want to quickly port the old NCE layer into the new framework.

// set dims of output(Out)
std::vector<int64_t> out_dims;
out_dims.push_back(x_dims[0]);
ctx->SetOutputDim("Cost", framework::make_ddim(out_dims));
Copy link
Contributor

Choose a reason for hiding this comment

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

The output shape is [batch_size, 1] in smooth_l1, cos_sim, sequared_l2_distance, softmax_with_cross_entropy, and this can be discussed and unified later.

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.

AddOutput("SampleLogits", "An intermediate tensor.").AsIntermediate();
AddOutput("SampleLabels", "An intermediate tensor.").AsIntermediate();
AddAttr<int>("num_classes", "Total number of classes.");
AddAttr<int>("num_sampled_classes", "The number of negative classes.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Might num_neg_samples be better if it indicates the number of negative samples for each positive sample. I just compare with the NCELayer, but I am not sure what num_sampled_classes represents for and which is better. Maybe I just haven't got the point.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with @guoshengCS . But maybe can use num_neg_classes to also incorporate @wanghaoshuang ‘s original consideration.

Copy link
Contributor Author

@wanghaoshuang wanghaoshuang Nov 24, 2017

Choose a reason for hiding this comment

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

Renamed num_sampled_classes to num_neg_samples

std::vector<int> sampled_labels =
ctx->Attrs().Get<std::vector<int>>("sampled_labels");
PADDLE_ENFORCE_EQ(num_classes, ctx->GetInputDim("Weight")[0]);
PADDLE_ENFORCE_LT(num_sampled_classes, num_classes);
Copy link
Contributor

Choose a reason for hiding this comment

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

If num_sampled_classes indicates the number of negative samples, the above PADDLE_ENFORCE_LT may not be needed.

Copy link
Contributor Author

@wanghaoshuang wanghaoshuang Nov 24, 2017

Choose a reason for hiding this comment

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

num_classes means the total number of classes in all samples. So i think this checking is necessary.
如果sampled_classes中有重复的class, 是有可能num_sampled_classes > num_classes的, 也不能说噪声样本数量大于num_classes就是错的,但是从计算性能上考虑,噪声样本数量大于num_classes是没有必要的,所以有了这里的checking.
另外paddle v2和tesoflow中都没有这个限制。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我还是先去掉这个checking吧

AddInput("Input", "(Tensor) A tensor of shape [batch_size, dim].");
AddInput("Label",
"(Tensor) A tensor of shape [batch_size, num_true_class]. "
"'num_true_class' is the number of target class in each sample.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this used for multi-label and must all samples have the same label number.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be useful to allow a variable number of target classes per example. For now, if you have a variable number of target classes, you can pad them out to a constant number by either repeating them or by padding with an otherwise unused class.

}
} else {
for (; j < sample_labels_dims[1]; ++j) {
sample_labels_data[index++] = rand(rng);
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that uniform distribution is the only supported temporarily. TODO can be added for future work.

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.

auto input_mat = EigenMatrix<T>::From(*(context.Input<Tensor>("Input")));
auto weight_mat = EigenMatrix<T>::From(*(context.Input<Tensor>("Weight")));
for (size_t i = 0; i < sample_labels->numel(); ++i) {
Eigen::Tensor<float, 0, Eigen::RowMajor, Eigen::DenseIndex> result =
Copy link
Contributor

Choose a reason for hiding this comment

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

Can EigenScalar in the framework be used here. I am not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

EigenScalar will cause compiling error:

错误:请求从‘const Eigen::TensorReductionOp<Eigen::internal::SumReducer<float>, const Eigen::DimensionList<long int, 1ul>, const Eigen::TensorCwiseBinaryOp<Eigen::internal::scalar_product_op<const float, const float>, const Eigen::TensorChippingOp<-1l, const Eigen::TensorMap<Eigen::Tensor<const float, 2, 1, long int>, 0, Eigen::MakePointer> >, const Eigen::TensorChippingOp<-1l, const Eigen::TensorMap<Eigen::Tensor<const float, 2, 1, long int>, 0, Eigen::MakePointer> > >, Eigen::MakePointer>’转换到非标量类型‘paddle::operators::EigenScalar<float, 1, long int> {aka paddle::framework::EigenScalar<float, 1, long int>}’
               .sum();

.sum();
sample_out_data[i] += result(0);
// activation_->forward
sample_out_data[i] = (1. / (1. + exp(-sample_out_data[i])));
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe only output values of positive samples are needed to compute, and some computation can be reduced in future.

if (d_bias != nullptr) {
T* d_bias_data = d_bias->mutable_data<T>(context.GetPlace());
for (size_t i = 0; i < sample_labels->numel(); ++i) {
d_bias_data[sample_labels_data[i]] += sample_grad_data[i];
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if the grad_data should be clear to zero first.

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.

samples.append((i, num, False, w))
sample_labels.append(num)
# forward bias
sampleOut = np.zeros(len(samples)).astype(np.float32)
Copy link
Contributor

Choose a reason for hiding this comment

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

sample_out might be better to unify name style.

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.

Copy link
Contributor

@lcy-seso lcy-seso left a comment

Choose a reason for hiding this comment

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

Some simple comments first.

"total number of class.");
AddInput("Bias",
"(Tensor) A tensor of shape [num_class]. 'num_class' is the total "
"number of class. It is a dispensable input.")
Copy link
Contributor

@lcy-seso lcy-seso Nov 21, 2017

Choose a reason for hiding this comment

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

I remember we have discussed before and decide to use the 2-dimensional tensor to represent a vector to distinguish it is a row vector or a column vector explicitly.

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.

AddOutput("SampleLogits", "An intermediate tensor.").AsIntermediate();
AddOutput("SampleLabels", "An intermediate tensor.").AsIntermediate();
AddAttr<int>("num_classes", "Total number of classes.");
AddAttr<int>("num_sampled_classes", "The number of negative classes.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with @guoshengCS . But maybe can use num_neg_classes to also incorporate @wanghaoshuang ‘s original consideration.

AddInput("SampleWeight",
"(Tensor) A tensor of shape [batch_size] storing a weight for "
"each sample. And it is a dispensable input. The default value of "
"sample is 1.")
Copy link
Contributor

Choose a reason for hiding this comment

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

The same problem as Input(Bias) about tensor's shape.

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.

"sample is 1.")
.AsDispensable();
AddOutput("Cost",
"(Tensor) A tensor of shape [batch_size]. Cost of samples.");
Copy link
Contributor

Choose a reason for hiding this comment

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

The same problem as Input(Bias) about tensor's shape.

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.

.SetDefault(10);
AddAttr<std::vector<int>>("sampled_labels", "");
AddComment(R"DOC(
Computes and returns the noise-contrastive estimation training loss.
Copy link
Contributor

Choose a reason for hiding this comment

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

Compute and return

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.

distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make the indention of license follows that in accuracy_op.h.

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.

AddOutput("Cost",
"(Tensor) A tensor of shape [batch_size]. Cost of samples.");
AddOutput("SampleLogits", "An intermediate tensor.").AsIntermediate();
AddOutput("SampleLabels", "An intermediate tensor.").AsIntermediate();
Copy link
Contributor

Choose a reason for hiding this comment

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

Comments on Output(SampleLogits) and Output(SampleLabels). What are these intermediate outputs used for?

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 by adding more comments.

PADDLE_ENFORCE(ctx->HasInput("SampleLogits"));
PADDLE_ENFORCE(ctx->HasInput("SampleLabels"));
PADDLE_ENFORCE(ctx->HasInput(framework::GradVarName("Cost")),
"The input(Out@GRAD) should not be null");
Copy link
Contributor

Choose a reason for hiding this comment

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

If the comment is a complete sentence, please add a period at the end of the sentence.

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.

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.

using framework::Tensor;

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.

public:
NCEOpMaker(framework::OpProto* proto, framework::OpAttrChecker* op_checker)
: OpProtoAndCheckerMaker(proto, op_checker) {
AddInput("Input", "(Tensor) A tensor of shape [batch_size, dim].");
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a question here. From the current implementation, this operator only supports uniform distribution which is hardcoded. How can we extend the codes to support more distribution? Should we make distribution method an attribute, or leave the sampling process outside this operator? What is your suggestion?

Copy link
Contributor Author

@wanghaoshuang wanghaoshuang Nov 27, 2017

Choose a reason for hiding this comment

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

Yes. You are right. More distribution sampler is necessary. The terms in NLP application always meet heavy-tailed distribution. So we need a long uniform distribution sampler. And the distribution`s PDF is 1/(x+1)lnR in which R is the range of sampling. I will implement the log uniform sampler and other common distribution samplers as independent math function in another pr.

1. Remove checking for num_neg_samples.
2. Fix dims of Output(Cost) and Input(Bias).
3. Renamed num_sampled_classes to num_neg_samples.
4. Add TODO for add more distribution sampler.
5. Init grad_data of bias by zero.
6. Refine comments.
7. Register a kernel for type double.
Copy link
Contributor

@lcy-seso lcy-seso left a comment

Choose a reason for hiding this comment

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

LGTM

distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License. */
Copy link
Contributor

Choose a reason for hiding this comment

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

The indentation should like that in nce_op.cc

@wanghaoshuang wanghaoshuang merged commit 10b2534 into PaddlePaddle:develop Dec 1, 2017
@wanghaoshuang wanghaoshuang deleted the nce_op branch May 20, 2022 03:56
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.

5 participants