Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
Adding MultiLabel SoftMargin Loss #2345
Adding MultiLabel SoftMargin Loss #2345
Changes from 50 commits
e1eb983
7269357
a3c3bb7
92af440
b2604b0
fc6335d
b6275ec
b42c360
90c48ae
c0a3ef6
194f3ed
c8280f1
6783aa7
71e1a89
43d3506
a346c1a
aa4b596
503185b
1c43613
b06486f
67ec433
6ec768f
ab55a5e
697944c
70eb8bc
81fece7
9521457
ba11c43
3089f63
9e4da86
17806e0
e2eef8a
0ddda95
86fec68
43d91db
1c1d33d
c395ed3
eb17f31
dcd02d9
a450f71
56ce6fa
f298c7a
9789fde
d3068cc
4782809
4541966
6470ae8
34153d5
c5a24d6
9b2a3ac
6340e64
77463d8
733db6a
180f917
ebaa6c3
bf61e97
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be easier to understand what happens here, if we provide a second constructor to set the weights. Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi zoq, if you look at commit f298c7a, on May 5th, there I re-designed the API of this function to the current format after a long discussion above with @kartikdutt18. The conclusion of that discussion can be seen in this comment.
So, I think initially I had done what you are suggesting (let me know if you mean otherwise) using a overloaded constructor to set the weights. But then Kartik wanted me to design the API like this.
I am okay with both. Even though I would prefer the approach with an overloaded constructor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I am guessing the other constructor would also have weights as param and probably a reduction also as param. It might ve redundant to have two constructors with same parameters. We would only have one forward function so we would have take into account both cases. For initialization we check if the vector is empty, if yes, we set reset to true and in forward pass we initialize the weights equal to input features and we set reset to false so we don't initialize every iteration. If weights are passed then we set reset to false and copy weights so no initialization in forward pass. Maybe I missed something, let me know what you think.
Regards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of API wouldn't an overloaded constructor be redundant. I am guessing one constructor will be with parameter reduction and one with both reduction and weights parameter. We can simply have one constructor and set weights to default value and in forward pass set them to 1 only in the first pass. This allows us to have a forward function which doesn't depend on which constructors object we are dealing with. I have left a comment explaining something similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could go into the other constructor, see comment above, that way we don't set this in each call of forward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree! Detailed comment above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I suggested using a reset parameter so this initialized only in the first pass. I am using my phone so sorry I can't take a look at the code. Similar thing is done in adaptive pooling layers. Regards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, The weights will be initialised every time in forward pass here, You should set weighted to true here to prevent that from happening. As shown here, we use reset to prevent weights from being initialized every time i.e. If user has not passed weights they will be initialized once in forward pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed! This was something I missed completely. Corrected in the commit 77463d8.
Thanks to both zoq and you for pointing this out.