Skip to content

Conversation

@nuka137
Copy link
Contributor

@nuka137 nuka137 commented Oct 7, 2019

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

@yf225
Copy link
Contributor

yf225 commented Oct 9, 2019

Thanks a lot for the great work! We will need some fixes as suggested in #27446.

@nuka137 nuka137 changed the title [WIP] C++ API: torch::nn::Softmax2d C++ API: torch::nn::Softmax2d Oct 11, 2019
@nuka137
Copy link
Contributor Author

nuka137 commented Oct 11, 2019

@yf225

#27446 is merged, so I removed WIP.
Could you review this PR again?

Copy link
Contributor

@yf225 yf225 left a comment

Choose a reason for hiding this comment

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

Thanks so much for the awesome work @nuka137! I left a very minor comment, and we should be able to merge it very soon :D

/// about the exact behavior of this module.
class TORCH_API Softmax2dImpl : public torch::nn::Cloneable<Softmax2dImpl> {
public:
Softmax2dImpl();
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove the default constructor because it will be created by default :D

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 it


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

Softmax2dImpl::Softmax2dImpl() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove the default constructor here :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I fixed it.

@nuka137
Copy link
Contributor Author

nuka137 commented Oct 12, 2019

@yf225

Thanks for reviewing.
I fixed all issues you mentioned.
Could you review again?

Copy link
Contributor

@yf225 yf225 left a comment

Choose a reason for hiding this comment

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

Thanks so much for the awesome work @nuka137!

@yf225
Copy link
Contributor

yf225 commented Oct 13, 2019

@pytorchbot rebase this please

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@facebook-github-bot
Copy link
Contributor

@yf225 merged this pull request in 07d4374.

thiagocrepaldi pushed a commit to thiagocrepaldi/pytorch that referenced this pull request Feb 4, 2020
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Merged module: cpp Related to C++ API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants