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

[MRG + 1] Fixes #7237 classes renamed to labels in hamming_loss #7260

Closed
wants to merge 14 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@srvanrell
Contributor

srvanrell commented Aug 26, 2016

Reference Issue

Fixes #7237

What does this implement/fix? Explain your changes.

classes is renamed to labels in hamming_loss().
A deprecation warning was added (to be removed in 0.20).

Any other comments?

PEP8 warnings in sklearn/metrics/classification.py were fixed

@@ -667,7 +667,8 @@ def f1_score(y_true, y_pred, labels=None, pos_label=1, average='binary',
References
----------
.. [1] `Wikipedia entry for the F1-score <https://en.wikipedia.org/wiki/F1_score>`_
.. [1] `Wikipedia entry for the F1-score

This comment has been minimized.

@amueller

amueller Aug 26, 2016

Member

does this render correctly?

This comment has been minimized.

@nelson-liu

nelson-liu Aug 27, 2016

Contributor

+1 I feel like it wouldn't

This comment has been minimized.

@srvanrell

srvanrell Aug 29, 2016

Contributor

I think it would. It's not different from docstrings of confusion_matrix() (line 215) and cohen_kappa_score() (line 324). I didn't change them and it seems that they are correctly rendered (see confusion_matrix and cohen_kappa_score)

This comment has been minimized.

@amueller

amueller Aug 29, 2016

Member

ok cool

@@ -1467,12 +1469,17 @@ def hamming_loss(y_true, y_pred, classes=None, sample_weight=None):
y_pred : 1d array-like, or label indicator array / sparse matrix
Predicted labels, as returned by a classifier.
classes : array, shape = [n_labels], optional
Integer array of labels.
labels : array, shape = [n_labels], optional (default=None)

This comment has been minimized.

@amueller

amueller Aug 26, 2016

Member

should have a ..versionadded: tag (do a git grep)

This comment has been minimized.

@amueller

amueller Aug 29, 2016

Member

(still missing)

This comment has been minimized.

@amueller

amueller Aug 29, 2016

Member

whoops sorry needed to refresh

@@ -1498,6 +1505,9 @@ def hamming_loss(y_true, y_pred, classes=None, sample_weight=None):
The Hamming loss is upperbounded by the subset zero-one loss. When
normalized over samples, the Hamming loss is always between 0 and 1.
'classes' was renamed to 'labels' in version 0.18 and will be removed in

This comment has been minimized.

@amueller

amueller Aug 26, 2016

Member

this should go into the whatsnew.

@amueller

This comment has been minimized.

Member

amueller commented Aug 26, 2016

You need to change the tests, and add a test that the deprecation warning is actually raised. Otherwise looks good. Thanks :)

@amueller

This comment has been minimized.

Member

amueller commented Aug 26, 2016

(There might also be doctests to fix, let's see if the CI barks)

@srvanrell

This comment has been minimized.

Contributor

srvanrell commented Aug 26, 2016

I'll check it and update what is necessary ;)

@@ -380,6 +380,11 @@ Bug fixes
- Fix :class:`linear_model.ElasticNet` sparse decision function to match
output with dense in the multioutput case.
- `classes` parameter was renamed to `labels` in

This comment has been minimized.

@amueller

amueller Aug 26, 2016

Member

double backticks for code literals

@amueller

This comment has been minimized.

Member

amueller commented Aug 26, 2016

great, the tests still need changing (so they don't throw deprecation warnings), and a test for the warning needs to be added.

@srvanrell

This comment has been minimized.

Contributor

srvanrell commented Aug 26, 2016

Tests don't use parameter classes nor labels. Should I look for examples to add a proper test?

@amueller

This comment has been minimized.

Member

amueller commented Aug 26, 2016

can you add hamming to the METRICS_WITH_LABELS in metrics/tests/test_common.py?

@amueller

This comment has been minimized.

Member

amueller commented Aug 26, 2016

LGTM if tests pass. thanks :)

@amueller amueller changed the title from [MRG] Fixes #7237 classes renamed to labels in hamming_loss to [MRG + 1] Fixes #7237 classes renamed to labels in hamming_loss Aug 26, 2016

sample_weight : array-like of shape = [n_samples], optional
Sample weights.
classes : array, shape = [n_labels], optional
(deprecated) Integer array of labels. This parameter has been
renamed to ``labels`` in version 0.18 an will be removed in 0.20.

This comment has been minimized.

@nelson-liu

nelson-liu Aug 27, 2016

Contributor

an -> and

@srvanrell

This comment has been minimized.

Contributor

srvanrell commented Aug 27, 2016

If anything else have to be done just let me know.

@@ -380,6 +380,11 @@ Bug fixes
- Fix :class:`linear_model.ElasticNet` sparse decision function to match
output with dense in the multioutput case.
- ``classes`` parameter was renamed to ``labels`` in

This comment has been minimized.

@jnothman

jnothman Aug 27, 2016

Member

This should be under API changes.

labels : array, shape = [n_labels], optional (default=None)
Integer array of labels. If not provided, labels will be inferred
from y_true and y_pred.
.. versionadded:: 0.18

This comment has been minimized.

@jnothman

jnothman Aug 27, 2016

Member

I think you might need a blank line before this.

srvanrell added some commits Aug 27, 2016

@@ -414,6 +414,11 @@ API changes summary
(`#7187 <https://github.com/scikit-learn/scikit-learn/pull/7187>`_)
by `YenChen Lin`_.
- ``classes`` parameter was renamed to ``labels`` in
:func:`metrics.classification.hamming_loss`.
(`#7237 <https://github.com/scikit-learn/scikit-learn/issues/7237>`_) by

This comment has been minimized.

@nelson-liu

nelson-liu Aug 27, 2016

Contributor

I think most people link the pull request instead of the issue

@srvanrell

This comment has been minimized.

Contributor

srvanrell commented Aug 27, 2016

Done.

I don't really know how to do it but it might be necessary to add a test for the deprecation warning.

- ``classes`` parameter was renamed to ``labels`` in
:func:`metrics.classification.hamming_loss`.
(`#7260 <https://github.com/scikit-learn/scikit-learn/pull/7260>`_) by
`Sebastian Vanrell`_.

This comment has been minimized.

@nelson-liu

nelson-liu Aug 27, 2016

Contributor

you probably want to add a link definition for your name at the bottom of the file

@nelson-liu

This comment has been minimized.

Contributor

nelson-liu commented Aug 27, 2016

@srvanrell

This comment has been minimized.

Contributor

srvanrell commented Aug 27, 2016

Thanks, I'll try to add a test

@nelson-liu

This comment has been minimized.

Contributor

nelson-liu commented Aug 30, 2016

this LGTM too. thanks @srvanrell

@@ -767,6 +767,7 @@ def test_multilabel_hamming_loss():
assert_equal(hamming_loss(y1, np.zeros_like(y1), sample_weight=w), 2. / 3)
# sp_hamming only works with 1-D arrays
assert_equal(hamming_loss(y1[0], y2[0]), sp_hamming(y1[0], y2[0]))
assert_warns(DeprecationWarning, hamming_loss, y1, y2, classes=np.array([0, 1]))

This comment has been minimized.

@jnothman

jnothman Aug 30, 2016

Member

pep8 line lemgth

@@ -767,6 +768,7 @@ def test_multilabel_hamming_loss():
assert_equal(hamming_loss(y1, np.zeros_like(y1), sample_weight=w), 2. / 3)
# sp_hamming only works with 1-D arrays
assert_equal(hamming_loss(y1[0], y2[0]), sp_hamming(y1[0], y2[0]))
assert_warns(DeprecationWarning, hamming_loss, y1, y2, classes=l)

This comment has been minimized.

@jnothman

jnothman Aug 30, 2016

Member

I should have suggested that you just do classes=[0,1].

This comment has been minimized.

@srvanrell

srvanrell via email Aug 30, 2016

Contributor
@jnothman

This comment has been minimized.

Member

jnothman commented Aug 30, 2016

Merged as e2648b1

@jnothman jnothman closed this Aug 30, 2016

@jnothman

This comment has been minimized.

Member

jnothman commented Aug 30, 2016

Thanks @srvanrell

@srvanrell

This comment has been minimized.

Contributor

srvanrell commented Aug 30, 2016

Thanks all of you for your help.

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