Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
nuka137 wants to merge 6 commits into from
Closed

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

nuka137 wants to merge 6 commits into from

Conversation

@nuka137
Copy link
Contributor

@nuka137 nuka137 commented Oct 7, 2019 •
edited
Loading

Add torch::nn::Softmin module and functional support for the C++ API.

Related Issue: #25883

Reviewer: @yf225

@pytorchbot pytorchbot added the module: cpp Related to C++ API label Oct 7, 2019
@nuka137 nuka137 changed the title [WIP] C++ API: torch::nn::Softmin C++ API: torch::nn::Softmin Oct 7, 2019
@yf225 yf225 mentioned this pull request Oct 7, 2019
Open
@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
Copy link
Contributor Author

nuka137 commented Oct 9, 2019

@yf225

Fixed all issues discussed in #27446, and rebased to newest master branch.
Could you review this PR again?

yf225
yf225 reviewed Oct 9, 2019
View reviewed changes
torch/csrc/api/include/torch/nn/options/activation.h Outdated
struct TORCH_API SoftminOptions {
SoftminOptions(int64_t dim);

// Dimension along which Softmin will be computed.
Copy link
Contributor

@yf225 yf225 Oct 9, 2019

Choose a reason for hiding this comment

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

nit: we should change // to /// so that the comment will show up in the C++ API docs (my apologies for missing it for the Softmax module 😞)

Copy link
Contributor Author

@nuka137 nuka137 Oct 9, 2019

Choose a reason for hiding this comment

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

@yf225

If you don't mind, I'll also fix Softmax module in this PR.
What do you think about this idea?

Copy link
Contributor

@yf225 yf225 Oct 9, 2019

Choose a reason for hiding this comment

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

Yes that would be awesome, thanks so much for your help @nuka137!

Copy link
Contributor Author

@nuka137 nuka137 Oct 9, 2019

Choose a reason for hiding this comment

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

Fixed them.

@nuka137
Copy link
Contributor Author

nuka137 commented Oct 9, 2019

@yf225

I fixed the comments on SoftminOptions and SoftmaxOptions.
Could you check them?
And I noticed that there are conflicts, do I need some work against them?

yf225
yf225 approved these changes Oct 9, 2019
View reviewed changes
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.

@nuka137 The changes look awesome, thanks so much for your help! I will fix the merge conflicts :D

facebook-github-bot
facebook-github-bot reviewed Oct 12, 2019
View reviewed changes
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

facebook-github-bot commented Oct 12, 2019

@yf225 merged this pull request in abaa441.

thiagocrepaldi pushed a commit to thiagocrepaldi/pytorch that referenced this pull request Feb 4, 2020
@nuka137
C++ API: torch::nn::Softmin (pytorch#27459)
b8bb5f6 
Summary:
Add torch::nn::Softmin module and functional support for the C++ API.

Related Issue: pytorch#25883

Reviewer: yf225
Pull Request resolved: pytorch#27459

Differential Revision: D17892852

Pulled By: yf225

fbshipit-source-id: db15b06e8ad33947e7d65995df700f5e90c3b6a8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@yf225 yf225 yf225 approved these changes

@ebetica ebetica Awaiting requested review from ebetica

@goldsborough goldsborough Awaiting requested review from goldsborough

Assignees

No one assigned

Labels

Merged module: cpp Related to C++ API

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@nuka137 @yf225 @facebook-github-bot @pytorchbot @mruberry