C++ API: torch::nn::Softmax #27446

nuka137 · 2019-10-06T14:25:24Z

Add torch::nn::Softmax module support for the C++ API

Related Issue: #25883

Reviewer: @yf225

pbelevich · 2019-10-06T17:31:25Z

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

+/// Options for the `Softmax` module.
+struct TORCH_API SoftmaxOptions {
+  // Dimension along which Softmax will be computed.
+  TORCH_ARG(int, dim);


Please add a constructor with dim parameter taking default value, as @yf225 asked here, see HardshrinkOptions as an example

I suggest to have -1 as a default value

Thanks. I fixed it.

After discussion with the team we agreed that we should use c10::optional<int64_t> instead of just int here. The default value should be c10::nullopt. I'm sorry for for misleading you.

@pbelevich

No problem. Thanks for the instruction.
Does this rule also apply to torch::Dtype ?

dim should be a non-optional argument because allowing it to be null is already deprecated in Python version, and the C++ version should just match the newest Python version behavior.

Thanks, I fixed it to make dim non-optional argument.

test/cpp/api/modules.cpp

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

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

test/cpp/api/modules.cpp

nuka137 · 2019-10-06T23:27:45Z

@pbelevich

Thanks for reviewing this PR.
I fixed all comments you reviewed.

pbelevich · 2019-10-07T16:36:12Z

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

+
+  if (dim == -1) {
+    int input_dim = input.dim();
+    if (input_dim == 0 || input_dim == 1 || input_dim == 3) {


Please extract this to _get_softmax_dim(...) as it is in python and reuse it in LogSoftmax. Use TORCH_WARN for the warning.

I fixed it.

pbelevich · 2019-10-07T16:43:58Z

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

+/// Options for the `Softmax` module.
+struct TORCH_API SoftmaxOptions {
+  // Dimension along which Softmax will be computed.
+  TORCH_ARG(int, dim);


After discussion with the team we agreed that we should use c10::optional<int64_t> instead of just int here. The default value should be c10::nullopt. I'm sorry for for misleading you.

yf225

@nuka137 Thanks so much for the great contribution! I left some comments mostly regarding the design of SoftmaxOptions.

yf225 · 2019-10-08T05:32:36Z

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

+    ret = 1;
+  }
+  return ret;
+}


In the Python version, this function is for maintaining backward compatibility for the use cases where people don't pass the dim argument to torch.nn.functional.softmax. Since torch::nn::functional::softmax in C++ API is a new addition, we don't need to maintain any backward compatibility, and we should remove this function and make dim a non-optional argument to match the newest Python API design.

I understood the background and deleted the function _get_softmax_dim.

yf225 · 2019-10-08T05:38:30Z

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

+  // The desired data type of returned tensor.
+  // If specified, the input tensor is casted to dtype before the operation
+  // is performed. This is useful for preventing data type overflows.
+  TORCH_ARG(torch::Dtype, dtype);


I think semantically dtype is not one of the options of a softmax function/module, because it doesn't control how the softmax function itself should perform the computation, but rather how the returned tensor should be stored, which is rather mechanical than mathematical.

OK, deleted dtype from SoftmaxOptions.

yf225 · 2019-10-08T05:40:31Z

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

+/// Options for the `Softmax` module.
+struct TORCH_API SoftmaxOptions {
+  // Dimension along which Softmax will be computed.
+  TORCH_ARG(int, dim);


dim should be a non-optional argument because allowing it to be null is already deprecated in Python version, and the C++ version should just match the newest Python version behavior.

yf225 · 2019-10-08T05:42:01Z

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

+
+/// Options for the Softmax functional and module.
+struct TORCH_API SoftmaxOptions {
+  SoftmaxOptions(int dim = -1, torch::Dtype dtype = torch::Dtype::Undefined);


I think it should be

Suggested change

SoftmaxOptions(int dim = -1, torch::Dtype dtype = torch::Dtype::Undefined);

SoftmaxOptions(int dim);

because dim shouldn't be an optional argument.

Thanks for your suggestion. I fixed it.

yf225 · 2019-10-08T05:43:17Z

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

+  if (dtype != torch::Dtype::Undefined) {
+    stream << "dtype=" << options.dtype();
+  }
+  stream << ")";


To match the Python version, we can just print dim in pretty_print.

Fixed it to print dim only.

yf225 · 2019-10-08T05:45:59Z

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

+  return ret;
+}
+
+inline Tensor softmax(const Tensor& input, const SoftmaxOptions& options) {


I think the signature should be

Suggested change

inline Tensor softmax(const Tensor& input, const SoftmaxOptions& options) {

inline Tensor softmax(const Tensor& input, const SoftmaxOptions& options, c10::optional<torch::Dtype> dtype = c10::nullopt) {

Thanks for your suggestion. I fixed it.

nuka137 · 2019-10-08T12:04:22Z

@yf225

Thanks for reviewing.
I fixed all issues. Could you check this PR again?

yf225 · 2019-10-09T06:10:37Z

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

+  SoftmaxOptions(int dim);
+
+  // Dimension along which Softmax will be computed.
+  TORCH_ARG(int, dim);


I think we should make dim have int64_t type, because that's the type of tensor dimensions for C++ tensors.

OK, fixed it

yf225 · 2019-10-09T06:12:23Z

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

+/// about the exact behavior of this module.
+class TORCH_API SoftmaxImpl : public torch::nn::Cloneable<SoftmaxImpl> {
+ public:
+  explicit SoftmaxImpl(const SoftmaxOptions& options_);


We should also add explicit SoftmaxImpl(int64_t dim) : SoftmaxImpl(SoftmaxOptions(dim)) {} here, to serve the torch::nn::Softmax(/*dim=*/3) use case.

yf225 · 2019-10-09T06:13:21Z

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

+  int dim = options.dim();
+  Tensor ret;
+
+  if (dtype == torch::Dtype::Undefined) {


We should change this to

Suggested change

if (dtype == torch::Dtype::Undefined) {

if (dtype == c10::nullopt) {

Sorry, I missed this part. Now, it was fixed.

yf225 · 2019-10-09T06:14:31Z

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


+inline Tensor softmax(const Tensor& input, const SoftmaxOptions& options,
+                      c10::optional<torch::Dtype> dtype = c10::nullopt) {
+  int dim = options.dim();


We should change here accordingly:

Suggested change

int dim = options.dim();

int64_t dim = options.dim();

Thanks. I fixed it.

yf225 · 2019-10-09T06:16:25Z

test/cpp/api/functional.cpp

+
+TEST_F(FunctionalTest, Softmax) {
+  auto input = torch::arange(10, torch::kFloat).reshape({2, 5});
+  auto output = F::softmax(input, SoftmaxOptions(1));


We can change this to

Suggested change

auto output = F::softmax(input, SoftmaxOptions(1));

auto output = F::softmax(input, /*dim=*/1);

to also test SoftmaxOptions's implicit constructor. ;)

yf225 · 2019-10-09T06:17:00Z

test/cpp/api/modules.cpp

 }

+TEST_F(ModulesTest, Softmax) {
+  Softmax m(SoftmaxOptions(1));


We can change this to

Suggested change

Softmax m(SoftmaxOptions(1));

Softmax m(/*dim=*/1);

to also test SoftmaxOptions's implicit constructor. ;)

This is also fixed as well.

yf225 · 2019-10-09T06:18:47Z

@nuka137 I think similar issues in this PR also exist in Softmin / LogSoftmax / Softmax2d, and it would be awesome to change them as well. Thanks so much for the fantastic work!

nuka137 · 2019-10-09T13:06:06Z

@yf225

Fixed again all comments you reviewed.

similar issues in this PR also exist in Softmin / LogSoftmax / Softmax2d

Sure. I will change these PR when this PR is merged.
Thanks.

yf225

Thanks so much for the awesome work @nuka137! I will merge this today :D

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 · 2019-10-10T01:46:29Z

@yf225 merged this pull request in 28a1806.

Summary: Add torch::nn::Softmax2d module support for the C++ API. Softmax2d only supports module in Python API, so this PR adds only module support as well. This PR is WIP because it uses the function in #27446 . After #27446 is merged, I will remove WIP. Related Issue: #25883 Reviewer: yf225 Pull Request resolved: #27509 Differential Revision: D17899715 Pulled By: yf225 fbshipit-source-id: bd891bc995f5a92bf4f5405f8bf07d1bd5de2479

Summary: Add torch::nn::Softmax module support for the C++ API Related Issue: pytorch#25883 Reviewer: yf225 Pull Request resolved: pytorch#27446 Differential Revision: D17839546 Pulled By: yf225 fbshipit-source-id: 7c7fb55111b261614de7c3a75fa1019fbde93c67

Summary: Add torch::nn::Softmax2d module support for the C++ API. Softmax2d only supports module in Python API, so this PR adds only module support as well. This PR is WIP because it uses the function in pytorch#27446 . After pytorch#27446 is merged, I will remove WIP. Related Issue: pytorch#25883 Reviewer: yf225 Pull Request resolved: pytorch#27509 Differential Revision: D17899715 Pulled By: yf225 fbshipit-source-id: bd891bc995f5a92bf4f5405f8bf07d1bd5de2479

C++ API: torch::nn::Softmax

9bcf5dd

nuka137 requested review from ebetica, goldsborough and yf225 as code owners October 6, 2019 14:25

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

yf225 mentioned this pull request Oct 6, 2019

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

Open

pbelevich suggested changes Oct 6, 2019

View reviewed changes

Apply review comments

25a843a

nuka137 mentioned this pull request Oct 7, 2019

C++ API: torch::nn::Softmin #27459

Closed

pbelevich self-requested a review October 7, 2019 16:04

pbelevich suggested changes Oct 7, 2019

View reviewed changes

Apply review comments

6134a6a

nuka137 mentioned this pull request Oct 7, 2019

C++ API: torch::nn::Softmax2d #27509

Closed

yf225 requested changes Oct 8, 2019

View reviewed changes

Apply review comments

4d25a67

yf225 reviewed Oct 9, 2019

View reviewed changes

Apply review comments

a50c1ba

yf225 approved these changes Oct 9, 2019

View reviewed changes

facebook-github-bot reviewed Oct 9, 2019

View reviewed changes

facebook-github-bot closed this in 28a1806 Oct 9, 2019

yf225 mentioned this pull request Oct 9, 2019

C++ API: torch::nn::LogSoftmax #27462

Closed

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

mruberry added the Merged label Oct 28, 2020

	SoftmaxOptions(int dim = -1, torch::Dtype dtype = torch::Dtype::Undefined);
	SoftmaxOptions(int dim);

	inline Tensor softmax(const Tensor& input, const SoftmaxOptions& options) {
	inline Tensor softmax(const Tensor& input, const SoftmaxOptions& options, c10::optional<torch::Dtype> dtype = c10::nullopt) {

	if (dtype == torch::Dtype::Undefined) {
	if (dtype == c10::nullopt) {

	auto output = F::softmax(input, SoftmaxOptions(1));
	auto output = F::softmax(input, /dim=/1);

Uh oh!

C++ API: torch::nn::Softmax #27446

C++ API: torch::nn::Softmax #27446

Uh oh!

Conversation

nuka137 commented Oct 6, 2019

Uh oh!

pbelevich Oct 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nuka137 commented Oct 6, 2019

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 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!

Choose a reason for hiding this comment

Uh oh!

nuka137 commented Oct 8, 2019

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

pbelevich Oct 6, 2019 •

edited

Loading

nuka137 Oct 9, 2019 •

edited

Loading