Adding MSELoss, KLDivLoss and BCELoss to C++ front-end #27156

ShahriarRezghi · 2019-10-01T19:36:17Z

This PR adds MSELoss, KLDivLoss and BCELoss. The tests for BCELoss fail with the following error:

unknown file: Failure
C++ exception with description "autograd_meta() INTERNAL ASSERT FAILED at /home/shahriar/Contrib/pytorch/c10/core/TensorImpl.h:533, please report a bug to PyTorch. set_requires_grad is not implemented for Tensor (set_requires_grad at /home/shahriar/Contrib/pytorch/c10/core/TensorImpl.h:533)

yf225

@ShahriarSS Thanks a lot for the great work and my sincere apologies for the delay. I left some comments.

yf225 · 2019-10-09T05:08:14Z

torch/csrc/api/include/torch/nn/options/loss.h

 // ============================================================================

+/// Options for a KLDiv loss module.
+using KLDivLossOptions = L1LossOptions;


I think we should define all the new loss options explicitly instead of aliasing L1LossOptions, because L1LossOptions could take a different default reduction value or get a new options arg one day, and we might not want those changes to affect the other loss options.

yf225 · 2019-10-09T05:10:16Z

torch/csrc/api/include/torch/nn/functional/loss.h

+    const Tensor& target,
+    const MSELossOptions& options) {
+  return torch::mse_loss(self, target, options.reduction());
+}


It would be awesome to add tests for the new functionals as well :D

yf225 · 2019-10-09T05:12:20Z

torch/csrc/api/include/torch/nn/options/loss.h

+// ============================================================================
+
+/// Options for a BCE loss module.
+using BCELossOptions = L1LossOptions;


I think BCELossOptions can take weight and reduction as constructor args, so it's not strictly equivalent to L1LossOptions.

yf225 · 2019-10-09T05:13:16Z

torch/csrc/api/src/nn/modules/loss.cpp

+}
+
+Tensor KLDivLossImpl::forward(const Tensor& input, const Tensor& target) {
+  return torch::kl_div(input, target, options.reduction());


I think we should call F::kl_div here

yf225 · 2019-10-09T05:13:31Z

torch/csrc/api/src/nn/modules/loss.cpp

+}
+
+Tensor MSELossImpl::forward(const Tensor& input, const Tensor& target) {
+  return torch::mse_loss(input, target, options.reduction());


ditto for F::mse_loss

yf225 · 2019-10-09T05:21:58Z

torch/csrc/api/include/torch/nn/functional/loss.h

 namespace functional {

+inline Tensor l1_loss(
+    const Tensor& self,


nit: input instead of self, to match Python version better :)

yf225 · 2019-10-09T05:22:16Z

torch/csrc/api/include/torch/nn/functional/loss.h

+}
+
+inline Tensor kl_div(
+    const Tensor& self,


ditto: self -> input

yf225 · 2019-10-09T05:22:20Z

torch/csrc/api/include/torch/nn/functional/loss.h

+}
+
+inline Tensor mse_loss(
+    const Tensor& self,


ditto: self -> input

yf225 · 2019-10-09T05:22:36Z

torch/csrc/api/include/torch/nn/functional/loss.h

 inline Tensor hinge_embedding_loss(
-    const Tensor& x1,
-    const Tensor& x2,
+    const Tensor& self,


ditto: self -> input

yf225 · 2019-10-09T05:23:13Z

torch/csrc/api/include/torch/nn/functional/loss.h

-    const Tensor& x2,
+    const Tensor& self,
+    const Tensor& target,
    const HingeEmbeddingLossOptions& options) {


ditto for options = {}

Summary: C++ API `Module::register_parameter` should accept undefined Tensor as parameter, which is equivalent to `module.register_parameter("param", None)` in Python API. This unblocks #26082 and #27156. Pull Request resolved: #27948 Differential Revision: D17931739 Pulled By: yf225 fbshipit-source-id: 21bdfc88e66e3dc39f3caf608a6a3de48c510fa9

yf225 · 2019-10-15T19:13:24Z

@pytorchbot rebase this please

yf225

@ShahriarSS Thanks a lot for the awesome work! I left some minor comments.

yf225 · 2019-10-15T19:17:40Z

torch/csrc/api/include/torch/nn/modules/loss.h

+
+/// Creates a criterion that measures the Binary Cross Entropy
+/// between the target and the output.
+struct TORCH_API BCELossImpl : Module {


I think all Impl classes need to subclass from public Cloneable<ImplName> and implement the void reset() override method, otherwise module->clone() won't work on them.

yf225 · 2019-10-15T19:22:03Z

torch/csrc/api/include/torch/nn/options/loss.h

+  BCELossOptions(
+      Tensor weight = {},
+      Reduction::Reduction reduction = Reduction::Mean)
+      : weight_(weight), reduction_(reduction) {}


I think as a convention we should only provide a non-default constructor when the options has non-optional arguments or when the options has only one argument. For BCELossOptions I think we can follow the design of HingeEmbeddingLossOptions by providing defaults to weight and reduction and removing the non-default constructor.

yf225 · 2019-10-15T19:23:11Z

torch/csrc/api/src/nn/modules/loss.cpp

+}
+
+void BCELossImpl::pretty_print(std::ostream& stream) const {
+  stream << "torch::nn::BCELoss";


It would be fantastic to add tests for the pretty_prints as well :D

To better match the Python version, we might need torch::nn::BCELoss()

yf225 · 2019-10-15T19:24:15Z

torch/csrc/api/src/nn/modules/loss.cpp

+    : options(options_) {}
+
+void KLDivLossImpl::pretty_print(std::ostream& stream) const {
+  stream << "torch::nn::KLDivLoss";


To better match the Python version, we might need torch::nn::KLDivLoss()

yf225 · 2019-10-15T19:24:29Z

torch/csrc/api/src/nn/modules/loss.cpp

+MSELossImpl::MSELossImpl(const MSELossOptions& options_) : options(options_) {}
+
+void MSELossImpl::pretty_print(std::ostream& stream) const {
+  stream << "torch::nn::MSELoss";


To better match the Python version, we might need torch::nn::MSELoss()

ShahriarRezghi · 2019-10-16T10:42:39Z

@yf225 Should we do anything in BCELoss's reset function to options.weight()?

yf225 · 2019-10-16T18:47:52Z

torch/csrc/api/src/nn/modules/loss.cpp

+// ============================================================================
+
+BCELossImpl::BCELossImpl(const BCELossOptions& options_) : options(options_) {
+  register_parameter("weight", options.weight());


Thanks for the catch! Yes I think we should move this into reset()

Also it seems that it should be a buffer, not a parameter, based on the Python version:

self.register_buffer('weight', weight)

yf225

Thanks a lot for the awesome work @ShahriarSS!

facebook-github-bot

@yf225 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@yf225 is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@yf225 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2019-10-18T07:10:57Z

@yf225 merged this pull request in 91a260c.

…#27948) Summary: C++ API `Module::register_parameter` should accept undefined Tensor as parameter, which is equivalent to `module.register_parameter("param", None)` in Python API. This unblocks pytorch#26082 and pytorch#27156. Pull Request resolved: pytorch#27948 Differential Revision: D17931739 Pulled By: yf225 fbshipit-source-id: 21bdfc88e66e3dc39f3caf608a6a3de48c510fa9

Summary: This PR adds ```MSELoss```, ```KLDivLoss``` and ```BCELoss```. The tests for ```BCELoss``` fail with the following error: ``` unknown file: Failure C++ exception with description "autograd_meta() INTERNAL ASSERT FAILED at /home/shahriar/Contrib/pytorch/c10/core/TensorImpl.h:533, please report a bug to PyTorch. set_requires_grad is not implemented for Tensor (set_requires_grad at /home/shahriar/Contrib/pytorch/c10/core/TensorImpl.h:533) ``` Pull Request resolved: pytorch#27156 Differential Revision: D17960323 Pulled By: yf225 fbshipit-source-id: 84b8431064f2f573679c03a8d7994e3e2f81a4d1

Added the losses

b3568c2

ShahriarRezghi requested review from ebetica, goldsborough and yf225 as code owners October 1, 2019 19:36

pytorchbot added the module: cpp Related to C++ API label Oct 1, 2019

yf225 mentioned this pull request Oct 1, 2019

Python/C++ API Parity: torch.nn modules and functional #25883

Open

ezyang added the open source label Oct 1, 2019

yf225 reviewed Oct 9, 2019

View reviewed changes

zou3519 added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Oct 10, 2019

ShahriarRezghi changed the title ~~Added the losses~~ Adding MSELoss, KLDivLoss and BCELoss to C++ front-end Oct 14, 2019

Made the requested changes

0bff2d1

yf225 mentioned this pull request Oct 14, 2019

Allow passing undefined Tensor to Module::register_parameter #27948

Closed

Merge branch 'master' into c++-mseloss

0aef14e

yf225 reviewed Oct 15, 2019

View reviewed changes

ShahriarSS added 2 commits October 16, 2019 14:06

Made the changes

d52f738

Merge remote-tracking branch 'origin/c++-mseloss' into c++-mseloss

5c1a67a

ShahriarSS added 2 commits October 16, 2019 17:29

Fixed and error

0d34433

Fixed lint

31e9685

yf225 reviewed Oct 16, 2019

View reviewed changes

Made the changes

f059b1e

yf225 approved these changes Oct 16, 2019

View reviewed changes

facebook-github-bot reviewed Oct 16, 2019

View reviewed changes

clang-tidy

00133cd

facebook-github-bot reviewed Oct 18, 2019

View reviewed changes

facebook-github-bot closed this in 91a260c Oct 18, 2019

facebook-github-bot added the merged label Oct 18, 2019

mruberry added the Merged label Oct 28, 2020

Adding MSELoss, KLDivLoss and BCELoss to C++ front-end #27156

Adding MSELoss, KLDivLoss and BCELoss to C++ front-end #27156

Uh oh!

Conversation

ShahriarRezghi commented Oct 1, 2019

Uh oh!

yf225 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yf225 commented Oct 15, 2019

Uh oh!

yf225 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShahriarRezghi commented Oct 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yf225 left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Oct 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants