Skip to content

Conversation

@arjoly
Copy link
Member

@arjoly arjoly commented Mar 5, 2013

With the support of multilabel input for the zero_one_loss, accuracy_score and hamming_loss, there were 2 bugs with numpy 1.3:

  • np.setxor1d does not make unique by default (and so, there were some failures with redundant labels)
  • there were some failures with 0-dimensionality array

This patch intend to solve both of these issues.
For more information, see #1606 and jenkins console log.

Copy link
Member

Choose a reason for hiding this comment

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

typo: required

Copy link
Member

Choose a reason for hiding this comment

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

BTW, this typo is replicated several times.

@ogrisel
Copy link
Member

ogrisel commented Mar 5, 2013

I am not sure how np.setxor1d works, but if the tests pass, I am ok with merging this :)

@arjoly
Copy link
Member Author

arjoly commented Mar 6, 2013

I fix the typo!

@ogrisel
Copy link
Member

ogrisel commented Mar 6, 2013

@larsmans @amueller this looks good to me. Shall we merge?

Copy link
Member

Choose a reason for hiding this comment

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

maybe add a comment both are in list of labels format

Copy link
Member Author

Choose a reason for hiding this comment

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

Why both?

Copy link
Member

Choose a reason for hiding this comment

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

y_pred and y_true.

@amueller
Copy link
Member

amueller commented Mar 6, 2013

to me it looks a bit as if the updated part should be refactored into a separate function. otherwise it looks good.

@arjoly
Copy link
Member Author

arjoly commented Mar 6, 2013

If you want, I can backport numpy.setxor1d.

There are other part that could be simplify once #1643 will be merged.
I will do that in a future pr.

@amueller
Copy link
Member

amueller commented Mar 6, 2013

I rather thought about refactoring a larger part. Can we not just implement one of the functions in terms of the other?

@larsmans
Copy link
Member

larsmans commented Mar 6, 2013

If all the tests pass, +1 for merge.

@amueller
Copy link
Member

amueller commented Mar 6, 2013

ok,lets merge and refactor later.

Lars Buitinck notifications@github.com schrieb:

If all the tests pass, +1 for merge.


Reply to this email directly or view it on GitHub:
#1741 (comment)

Diese Nachricht wurde von meinem Android-Mobiltelefon mit K-9 Mail gesendet.

ogrisel added a commit that referenced this pull request Mar 6, 2013
[MRG] FIX numpy 1.3 issues with the new multilabel metrics
@ogrisel ogrisel merged commit 4376f73 into scikit-learn:master Mar 6, 2013
@ogrisel
Copy link
Member

ogrisel commented Mar 6, 2013

I pushed the green button. Hopefully jenkins will be happy.

@arjoly
Copy link
Member Author

arjoly commented Mar 7, 2013

jenkins is happy now !!!

@arjoly
Copy link
Member Author

arjoly commented Mar 7, 2013

I rather thought about refactoring a larger part. Can we not just implement one of the functions in terms of the other?

@amueller I though about it and decided to avoid memory copy each time the data aren't properly formatted.

@arjoly arjoly deleted the metrics-fix-np-1.3 branch March 7, 2013 07:44
@amueller
Copy link
Member

amueller commented Mar 7, 2013

Sorry, I don't think I understand the argument. Can you elaborate?

@arjoly
Copy link
Member Author

arjoly commented Mar 7, 2013

For the moment, there is one implementation of the measure per format type (multiclass, indicator-matrix, list-of-labels). If you write one implementation for all format, you will need to perform data transformation each time you aren't in the proper format.

@amueller
Copy link
Member

amueller commented Mar 7, 2013

What I meant was to do #1748, not refactor the format types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants