Skip to content
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

Update multiclass_hinge_loss docstring #619

Closed
wants to merge 2 commits into from
Closed

Update multiclass_hinge_loss docstring #619

wants to merge 2 commits into from

Conversation

kli-casia
Copy link
Contributor

According to the code,
I think the original description

L_i = \\max_{j \\not = p_i} (0, t_j - t_{p_i} + \\delta)

Maybe should change to this

L_i = \\max_{j \\not = t_i} (0, p_j - p_{t_i} + \\delta)

where L_i is the loss for the i th sample, t_i is the ground truth label of the i th sample.
p is the predicted probability vector of the i th sample.

Becasue in the original description, t_{p_i} is meaningless since p_i is the predicted probabilty which is a decimal.

@@ -250,9 +250,9 @@ def binary_hinge_loss(predictions, targets, binary=True, delta=1):


def multiclass_hinge_loss(predictions, targets, delta=1):
"""Computes the multi-class hinge loss between predictions and targets.
"""Computes the item-wise multi-class hinge loss between predictions and targets.
Copy link
Member

Choose a reason for hiding this comment

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

This makes the line too long. Please undo.

@f0k
Copy link
Member

f0k commented Jun 24, 2016

Can you squash this into a single commit? You'll need to clone the repository to your machine with git, then do git checkout patch-1, git reset --soft 9f10cad, git commit --amend --no-edit, and git push --force.

@f0k f0k added this to the v0.2 milestone Jun 24, 2016
@f0k
Copy link
Member

f0k commented Jun 15, 2017

Will merge from the command line (to squash the two commits) after #846.

@Sentient07
Copy link
Contributor

I'm not able to add @kli-nlpr 's fork of Lasagne to my remote. It shows unknown repository here (on the top). So I will open a new PR

@Sentient07
Copy link
Contributor

I think this PR can be closed? (Master seems to be same as the diff that this proposes)

@f0k
Copy link
Member

f0k commented Jul 31, 2017

No, master still has the wrong version: https://github.com/Lasagne/Lasagne/blob/master/lasagne/objectives.py#L305
Otherwise this PR would have been marked as conflicting.
But I'll merge this myself, no need for a new PR, thanks!

@f0k f0k self-assigned this Jul 31, 2017
@Sentient07
Copy link
Contributor

No, master still has the wrong version

I had made this change previously and forgot to commit. It hence was moving around my other branches as well. My bad, sorry

f0k added a commit to f0k/Lasagne that referenced this pull request Aug 1, 2017
Update multiclass_hinge_loss docstring
@f0k
Copy link
Member

f0k commented Aug 1, 2017

Merged manually. Closing.

@f0k f0k closed this Aug 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants