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.

Sign up for GitHub

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

Jump to bottom

add MAPE to regression metrics (fixes #691) #822

Merged
merged 4 commits into from
Apr 10, 2021
Merged

add MAPE to regression metrics (fixes #691) #822

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
49b9eb5
add MAPE to regression metrics (fixes #691)
jameslamb Apr 6, 2021
1142fcc
linting
jameslamb Apr 9, 2021
b05213b
fix compatibility with older scikit-learn
jameslamb Apr 9, 2021
99415c7
linting
jameslamb Apr 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions dask_ml/metrics/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
)
from .regression import ( # noqa
mean_absolute_error,
mean_absolute_percentage_error,
mean_squared_error,
mean_squared_log_error,
r2_score,
Expand Down
27 changes: 27 additions & 0 deletions dask_ml/metrics/regression.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,33 @@ def mean_absolute_error(
return result


@derived_from(sklearn.metrics)
def mean_absolute_percentage_error(
y_true: ArrayLike,
y_pred: ArrayLike,
sample_weight: Optional[ArrayLike] = None,
multioutput: Optional[str] = "uniform_average",
compute: bool = True,
) -> ArrayLike:
_check_sample_weight(sample_weight)
epsilon = np.finfo(np.float64).eps
mape = abs(y_pred - y_true) / da.maximum(y_true, epsilon)
output_errors = mape.mean(axis=0)

if isinstance(multioutput, str) or multioutput is None:
if multioutput == "raw_values":
if compute:
return output_errors.compute()
else:
return output_errors
else:
raise ValueError("Weighted 'multioutput' not supported.")
result = output_errors.mean()
if compute:
result = result.compute()
return result


@derived_from(sklearn.metrics)
def r2_score(
y_true: ArrayLike,
Expand Down
3 changes: 2 additions & 1 deletion tests/metrics/test_regression.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,13 @@
import dask_ml.metrics


@pytest.fixture(params=["mean_squared_error", "mean_absolute_error", "r2_score"])
@pytest.fixture(params=["mean_squared_error", "mean_absolute_error", "mean_absolute_percentage_error", "r2_score"])
Copy link
Contributor

@hristog hristog Apr 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
@pytest.fixture(params=["mean_squared_error", "mean_absolute_error", "mean_absolute_percentage_error", "r2_score"])
@pytest.fixture(
params=[
"mean_squared_error",
"mean_absolute_error",
"mean_absolute_percentage_error",
"r2_score",
]
)

Looks like black==19.10b0 isn't happy about the line length here.

Copy link
Contributor

@hristog hristog Apr 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Also, perhaps, it'd be nice if the correctness of the method was sanity-checked against its sklearn counterpart, just as it's done for some of the other metrics a bit further down in the same test file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like black==19.10b0 isn't happy about the line length here.

Ah ok. If dask-ml has chosen to pin to older versions of linters, then I think the non-conda option documented at https://ml.dask.org/contributing.html#style will be unreliable, since

dask-ml/setup.py

Line 29 in f5e5bb4

"black",
doesn't have a pin for things like black.

Once i switched to the conda instructions there, I got the expected diff. Updated in 1142fcc.

Also, perhaps, it'd be nice if the correctness of the method was sanity-checked against its sklearn counterpart, just as it's done for some of the other metrics a bit further down in the same test file.

Can you clarify what you want me to change? As far as I can tell, that is exactly what happens by adding mean_squared_percentage_error to the metric_pairs fixture. Every metric in that fixture is tested against its scikit-learn equivalent by

dask-ml/tests/metrics/test_regression.py

Line 37 in f5e5bb4

assert abs(result - expected) < 1e-5
.

Copy link
Contributor

@hristog hristog Apr 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Ah ok. If dask-ml has chosen to pin to older versions of linters, then I think the non-conda option documented at https://ml.dask.org/contributing.html#style will be unreliable

You're absolutely right! I've got a PR over at #813 waiting to be reviewed (for a couple of weeks now), and subsequently merged. It should improve the static-checking situation.

Every metric in that fixture is tested against its scikit-learn equivalent by

Indeed - ignore me about this one, please! I got confused that we should probably further introduce extra tests like the test_mean_squared_log_error one.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll bring up the question of whether the setup.py versions of the linters should be pinned, too, in #813.

def metric_pairs(request):
"""Pairs of (dask-ml, sklearn) regression metrics.

* mean_squared_error
* mean_absolute_error
* mean_absolute_percentage_error
* r2_score
"""
return (
Expand Down