Add C++ Frontend : triplet-margin-loss #27613
Conversation
PyExtreme
requested review from
ebetica,
goldsborough and
yf225
as code owners
yf225
left a comment
yf225
left a comment
There was a problem hiding this comment.
Thanks a lot for the great work @PyExtreme! I left some comments. I believe we should also rebase this PR on top of current master and fix the code conflicts.
test/cpp/api/modules.cpp
Outdated
| TEST_F(ModulesTest, PrettyPrintTripletMarginLoss) { | ||
| ASSERT_EQ( | ||
| c10::str(TripletMarginLoss(TripletMarginLossOptions().margin(3).p(2).eps(1e-06).swap(false))), | ||
| "torch::nn::TripletMarginLoss(margin=3, p=2, eps=1e-06, swap=false)"); |
There was a problem hiding this comment.
Is it supposed to also print reduction? I guess we don't have a nice string representation of the reduction enum at the moment, and please feel free to not print it for now. :D
There was a problem hiding this comment.
@yf225 , Thanks a lot for reviewing it. I have learnt a lot from it, gained confidence and got familiar with the pytorch codebase.
Will surely make the mentioned changes and fix the conflicts.
Thank you
yf225
left a comment
yf225
left a comment
There was a problem hiding this comment.
Thanks @PyExtreme! I left some comments.
| <<<<<<< HEAD | ||
| ======= | ||
| using namespace std | ||
| >>>>>>> Add C++ Frontend - triplet-margin-loss |
There was a problem hiding this comment.
We need to fix the code conflict here, and we also shouldn't do using namespace std.
| ", swap=" << options.swap() << | ||
| ======= | ||
| ", swap=" <<options.swap() << | ||
| >>>>>>> Add C++ Frontend - triplet-margin-loss |
There was a problem hiding this comment.
We need to fix the code conflict here.
| ======= | ||
| ", swap=" <<options.swap() << | ||
| >>>>>>> Add C++ Frontend - triplet-margin-loss | ||
| ", reduction=" << options.reduction() << ")"; |
There was a problem hiding this comment.
It seems confusing that we have logic for printing reduction here, but the actual pretty_print string doesn't have reduction. I suggest removing the reduction printing logic here for now, until we have better support for printing named enums :)
There was a problem hiding this comment.
Hi @yf225 , I mistakenly pushed these changes. Sorry for wasting your time, I was still working on the PR.
pytorchbot
added
module: build
|
Hi @yf225 , I had to create a new branch to tackle merge conflict since I am using cloud due to some limitations on my PC. Therefore, I don't have enough command there. Also, I have incorporated the changes you have put before here Here is the link to my new PR From next time, I will find a smoother method for fixing conflicts. Thank you |
Summary: Hi yf225 , I had to create a new branch to tackle merge conflict since I am using cloud due to some limitations on my PC. Therefore, I don't have enough command there. Also, I have incorporated the changes you have put before here #27613 Also, it would be great if you could recommend me some resources to work smmothly on GCP..:-D Thank you Pull Request resolved: #27713 Differential Revision: D17899695 Pulled By: yf225 fbshipit-source-id: eb6643223148774a5cbbd093bdcc5623872e5bba
Summary: Hi yf225 , I had to create a new branch to tackle merge conflict since I am using cloud due to some limitations on my PC. Therefore, I don't have enough command there. Also, I have incorporated the changes you have put before here pytorch#27613 Also, it would be great if you could recommend me some resources to work smmothly on GCP..:-D Thank you Pull Request resolved: pytorch#27713 Differential Revision: D17899695 Pulled By: yf225 fbshipit-source-id: eb6643223148774a5cbbd093bdcc5623872e5bba
4 participants
Hi @yf225 , Here is the C++ frontend API TripletMarginLoss implementation. I have got some branching issues and therefore am opening a pull request.
The link of our previous conversation is here and I have made all the changes as mentioned.
#27525