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

ENH: zero_division parameter for classification… #14900

Merged

Conversation

@marctorrellas
Copy link
Contributor

marctorrellas commented Sep 6, 2019

See issue #14876

What does this implement/fix? Explain your changes.

zero_division parameter for precision, recall, and friends

Any other comments?

3 possible values:

  • "warn": same behavior as before
  • 0, 1: remove the warnings and set value for the metrics to this when the metrics is ill-defined (see issue for more details and examples)

Just to clarify:

image

prec will be ZD if all predictions are negative
rec will be ZD if all labels are negative
f will be ZD if everything is negative

Note that if ZD = "warn" this means 0 + warning

Copy link
Member

jnothman left a comment

thanks for opening the PR. Sorry I'm not able to review immediately.

doc/whats_new/v0.21.rst Outdated Show resolved Hide resolved
sklearn/metrics/classification.py Outdated Show resolved Hide resolved
…into prec_rec_fscore_zero_division

# Conflicts:
#	sklearn/metrics/classification.py
#	sklearn/metrics/tests/test_classification.py

lost some stuff after merging, need a review
@jnothman

This comment has been minimized.

Copy link
Member

jnothman commented Sep 6, 2019

- Changed whats_new to 0.22
- F-score only warns if both prec and rec are ill-defined
- new private method to simplify _prf_divide
@marctorrellas marctorrellas changed the title [WIP] Issue 14876: zero_division parameter for classification metrics [MRG] Issue 14876: zero_division parameter for classification metrics Sep 7, 2019
@marctorrellas

This comment has been minimized.

Copy link
Contributor Author

marctorrellas commented Sep 7, 2019

thanks for opening the PR. Sorry I'm not able to review immediately.

Hi @jnothman , it's just my second PR to sklearn so I'm still learning :)

I'm having a problem with git, it says I have 9 files changed, but actually changed only 3. It's like it's comparing with a master from some days ago. For example this commit:

2119490

is in master but its changes appear in the diff. Can you guide me to fix this?

@@ -892,6 +903,12 @@ def f1_score(y_true, y_pred, labels=None, pos_label=1, average='binary',
sample_weight : array-like of shape = [n_samples], optional
Sample weights.
zero_division : string or int, default="warn"
Sets the behavior when there is a zero division. If set to
("warn"|0)/1, returns 0/1 when both precision and recall are zero

This comment has been minimized.

Copy link
@jnothman

jnothman Sep 9, 2019

Member

I don't think this notation is easy enough to read. How about 'Sets the value to return when blah blah. If "warn" (default), this acts like 0 but also raises a warning.'

This comment has been minimized.

Copy link
@marctorrellas

marctorrellas Sep 11, 2019

Author Contributor

wrote something similar, please check new version

@@ -1062,7 +1092,12 @@ def _prf_divide(numerator, denominator, metric, modifier, average, warn_for):
return result

# remove infs
result[mask] = 0.0
result[mask] = float(zero_division == 1)

This comment has been minimized.

Copy link
@jnothman

jnothman Sep 9, 2019

Member

this is obfuscated. I'd rather 0.0 if zero_division in ('warn', 0) else 1

This comment has been minimized.

Copy link
@marctorrellas

marctorrellas Sep 11, 2019

Author Contributor

done in new version

sklearn/metrics/classification.py Outdated Show resolved Hide resolved
sklearn/metrics/classification.py Outdated Show resolved Hide resolved
fbeta = my_assert(*tmp, y_true, y_pred, beta=beta,
average=average, zero_division=zero_division)

zero_division = float(zero_division == 1)

This comment has been minimized.

Copy link
@jnothman

jnothman Sep 9, 2019

Member

obfuscated

This comment has been minimized.

Copy link
@marctorrellas

marctorrellas Sep 11, 2019

Author Contributor

simplified in the new version with two separated tests

assert_array_almost_equal(r, [0, 0, 0], 2)
assert_array_almost_equal(f, [0, 0, 0], 2)
func = precision_recall_fscore_support
my_assert = (assert_warns if zero_division == "warn"

This comment has been minimized.

Copy link
@jnothman

jnothman Sep 9, 2019

Member

if you must do something like this, use functools.partial to capture the arguments too.

But I think tests must be very readable code, as the reader needs to be absolutely certain of their correctness to be confident that they in turn imply the corretness of the code.

This comment has been minimized.

Copy link
@marctorrellas

marctorrellas Sep 11, 2019

Author Contributor

simplified in the new version with two separated tests

fbeta = my_assert(*tmp, y_true, y_pred, beta=beta,
average=None, zero_division=zero_division)

zero_division = float(zero_division == 1)

This comment has been minimized.

Copy link
@jnothman

jnothman Sep 9, 2019

Member

This is obfuscated. I'd rather a clear, separate test checking the behaviour of zero_divison, than a tiny, unexplicit piece in a larger test.

This comment has been minimized.

Copy link
@marctorrellas

marctorrellas Sep 11, 2019

Author Contributor

simplified in the new version with two separated tests

- better docstrings
- more explicit use of zero_division value
<https://visualstudio.microsoft.com/de/downloads/>`_.
<https://visualstudio.microsoft.com/downloads/>`_.

.. warning::

This comment has been minimized.

Copy link
@jnothman

jnothman Sep 12, 2019

Member

You've done something strange in trying to merge in changes from master. Please try to merge in the latest master again

This comment has been minimized.

Copy link
@marctorrellas

marctorrellas Sep 12, 2019

Author Contributor

merged again master into my branch. Now only the 3 files appear

@marctorrellas

This comment has been minimized.

Copy link
Contributor Author

marctorrellas commented Sep 23, 2019

@jnothman any more comments?

Copy link
Member

jnothman left a comment

Thanks for the ping.

I don't think we currently test the return value (i.e. zero_division=1) except in the case that all the labels (true and pred) are negative... we don't seem to test zero_division=1 in the zero-sample_weight case either (though it is a pretty weird case).

@@ -2065,7 +2176,8 @@ def log_loss(y_true, y_pred, eps=1e-15, normalize=True, sample_weight=None,
y_true : array-like or label indicator matrix
Ground truth (correct) labels for n_samples samples.
y_pred : array-like of float, shape = (n_samples, n_classes) or (n_samples,)
y_pred : array-like of float, shape = (n_samples, n_classes) or
(n_samples,)

This comment has been minimized.

Copy link
@jnothman

jnothman Sep 24, 2019

Member

Please leave this as it was. Going over the line length is the best we can do really to render correctly in pydoc and Sphinx

@@ -1875,7 +2030,7 @@ def test_hinge_loss_multiclass_with_missing_labels():
np.clip(dummy_losses, 0, None, out=dummy_losses)
dummy_hinge_loss = np.mean(dummy_losses)
assert (hinge_loss(y_true, pred_decision, labels=labels) ==
dummy_hinge_loss)
dummy_hinge_loss)

This comment has been minimized.

Copy link
@jnothman

jnothman Sep 24, 2019

Member

Please do not change unrelated things. It makes your contribution harder to review and may introduce merge conflicts to other pull requests.

This comment has been minimized.

Copy link
@marctorrellas

marctorrellas Sep 25, 2019

Author Contributor

if I don't change this I have flake8 warning:

sklearn/metrics/tests/test_classification.py:1988:18: E127 continuation line over-indented for visual indent

This comment has been minimized.

Copy link
@jnothman

jnothman Sep 25, 2019

Member

Yes, I know that this is bad PEP8... we've considered black, but not clearly decided in its favour

weights="linear"), 0.9412, decimal=4)
assert_almost_equal(cohen_kappa_score(y1, y2,
weights="quadratic"), 0.9541, decimal=4)
assert_almost_equal(

This comment has been minimized.

Copy link
@jnothman

jnothman Sep 24, 2019

Member

Please do not change unrelated things. It makes your contribution harder to review and may introduce merge conflicts to other pull requests.

This comment has been minimized.

Copy link
@marctorrellas

marctorrellas Sep 25, 2019

Author Contributor

sorry for that. This formatting things are so annoying, have you considered black? it's really handy

assert_almost_equal(fbeta, 0)


def test_precision_recall_f1_no_labels_average_none():
@pytest.mark.parametrize('zero_division', [0, 1])

This comment has been minimized.

Copy link
@jnothman

jnothman Sep 24, 2019

Member

I don't think this is an exemplary use-case for parametrize given that you then need to handle the warnings case separately!

This comment has been minimized.

Copy link
@marctorrellas

marctorrellas Sep 25, 2019

Author Contributor

Given your previous comment:

This is obfuscated. I'd rather a clear, separate test checking the behaviour of zero_divison, than a tiny, unexplicit piece in a larger test.

I decided to separate this into two tests. I think it is a lot more readable. Otherwise, there are if's or the use of functools.partial. I can go back to previous version, but honestly, if we want readability I think this is better (maybe with better names)

assert_array_almost_equal(fbeta, [0, 0, 0], 2)


def test_prf_warnings():
@pytest.mark.parametrize('zero_division', ["warn"])

This comment has been minimized.

Copy link
@jnothman

jnothman Sep 24, 2019

Member

not sure how this helps

@jnothman

This comment has been minimized.

Copy link
Member

jnothman commented Sep 25, 2019

@marctorrellas

This comment has been minimized.

Copy link
Contributor Author

marctorrellas commented Sep 25, 2019

updated table in the description, it was wrong :(

prec will be ZD if all predictions are negative
rec will be ZD if all labels are negative
f will be ZD if everything is negative

- added tests for YTN or YPN to check prec/rec with zero_division value
- cleaner tests
@marctorrellas

This comment has been minimized.

Copy link
Contributor Author

marctorrellas commented Sep 25, 2019

Thanks for the ping.

I don't think we currently test the return value (i.e. zero_division=1) except in the case that all the labels (true and pred) are negative... we don't seem to test zero_division=1 in the zero-sample_weight case either (though it is a pretty weird case).

Added zero_division to a test where prec and rec both have their peculiar cases. I don't understand the last comment, do you mean passing the labels param with a label that is not present?

@jnothman

This comment has been minimized.

Copy link
Member

jnothman commented Sep 25, 2019

Copy link
Member

jnothman left a comment

Thank you. This is looking good!

Let's see what others think about this, including the parameter name which I think is still up for debate.

Copy link
Member

jnothman left a comment

Thank you. This is looking good!

Let's see what others think about this, including the parameter name which I think is still up for debate.

@marctorrellas

This comment has been minimized.

Copy link
Contributor Author

marctorrellas commented Sep 25, 2019

Thank you. This is looking good!

Let's see what others think about this, including the parameter name which I think is still up for debate.

Thanks!

Copy link
Member

thomasjpfan left a comment

Would on_zero_division be a better name?

sklearn/metrics/classification.py Outdated Show resolved Hide resolved
sklearn/metrics/classification.py Outdated Show resolved Hide resolved
sklearn/metrics/classification.py Outdated Show resolved Hide resolved
sklearn/metrics/classification.py Outdated Show resolved Hide resolved
sklearn/metrics/classification.py Outdated Show resolved Hide resolved
@marctorrellas

This comment has been minimized.

Copy link
Contributor Author

marctorrellas commented Sep 25, 2019

Would on_zero_division be a better name?

IMHO, it's as readable as zero_division so I would keep the shortest one, but I have no strong opinion about this

All the rest of changes your proposed have been applied

@thomasjpfan

This comment has been minimized.

Copy link
Member

thomasjpfan commented Oct 2, 2019

When one sets zero_division=1 is it obvious that it means: "If the denominator is zero, the value of this metric is 1"?

The logical is "If there is zero division then do something (warn, or set 0 or 1)". My concern is how just "zero_division" does not capture the "if..." part of the statement.

Maybe if_zero_division is better?

@jnothman

This comment has been minimized.

Copy link
Member

jnothman commented Oct 2, 2019

@marctorrellas

This comment has been minimized.

Copy link
Contributor Author

marctorrellas commented Oct 2, 2019

I prefer on_zero_division to if_zero_division...

For me just zero_division is fine to be honest, but if I have to choose one I would go for on_zero_division

Here: https://scikit-learn.org/stable/modules/generated/sklearn.model_selection.cross_validate.html

the error_score has a similar behavior. Maybe we want zero_division_score? (IMHO I find it too long. If a user has doubts given the name she should just check the docs...)

@jnothman

This comment has been minimized.

Copy link
Member

jnothman commented Oct 2, 2019

@marctorrellas

This comment has been minimized.

Copy link
Contributor Author

marctorrellas commented Oct 10, 2019

can we have a 4th opinion on this or just take a decision on this?

@thomasjpfan

This comment has been minimized.

Copy link
Member

thomasjpfan commented Oct 10, 2019

I am also fine with zero_division.

@marctorrellas marctorrellas requested a review from thomasjpfan Oct 12, 2019
@thomasjpfan

This comment has been minimized.

Copy link
Member

thomasjpfan commented Oct 12, 2019

Thank you @marctorrellas !

@thomasjpfan thomasjpfan changed the title [MRG] Issue 14876: zero_division parameter for classification metrics ENH: zero_division parameter for classification… Oct 12, 2019
@thomasjpfan thomasjpfan merged commit 7f079e3 into scikit-learn:master Oct 12, 2019
19 checks passed
19 checks passed
LGTM analysis: C/C++ No code changes detected
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No new or fixed alerts
Details
ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: doc Your tests passed on CircleCI!
Details
ci/circleci: doc artifact Link to 0/doc/_changed.html
Details
ci/circleci: doc-min-dependencies Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
codecov/patch 99.07% of diff hit (target 96.76%)
Details
codecov/project Absolute coverage decreased by -0.02% but relative coverage increased by +2.31% compared to 4e9f97d
Details
scikit-learn.scikit-learn Build #20191010.43 succeeded
Details
scikit-learn.scikit-learn (Linux py35_conda_openblas) Linux py35_conda_openblas succeeded
Details
scikit-learn.scikit-learn (Linux py35_ubuntu_atlas) Linux py35_ubuntu_atlas succeeded
Details
scikit-learn.scikit-learn (Linux pylatest_conda_mkl) Linux pylatest_conda_mkl succeeded
Details
scikit-learn.scikit-learn (Linux pylatest_pip_openblas_pandas) Linux pylatest_pip_openblas_pandas succeeded
Details
scikit-learn.scikit-learn (Linux32 py35_ubuntu_atlas_32bit) Linux32 py35_ubuntu_atlas_32bit succeeded
Details
scikit-learn.scikit-learn (Windows py35_pip_openblas_32bit) Windows py35_pip_openblas_32bit succeeded
Details
scikit-learn.scikit-learn (Windows py37_conda_mkl) Windows py37_conda_mkl succeeded
Details
scikit-learn.scikit-learn (macOS pylatest_conda_mkl) macOS pylatest_conda_mkl succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.