Add mean_squared_error functional metric
#515
Conversation
|
🚀 Deployed on https://deploy-preview-515--etna-docs.netlify.app |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #515 +/- ##
==========================================
+ Coverage 90.42% 90.53% +0.11%
==========================================
Files 247 247
Lines 16559 16568 +9
==========================================
+ Hits 14973 15000 +27
+ Misses 1586 1568 -18 ☔ View full report in Codecov by Sentry. |
etna/metrics/functional_metrics.py
Outdated
| @@ -41,6 +41,45 @@ def _get_axis_by_multioutput(multioutput: str) -> Optional[int]: | |||
| assert_never(multioutput_enum) | |||
|
|
|||
|
|
|||
| def mse_with_missing_handling(y_true: ArrayLike, y_pred: ArrayLike, multioutput: str = "joint") -> ArrayLike: | |||
There was a problem hiding this comment.
We should discuss naming and the fact that sklearn's mse is in our etna/metrics/__init__.py.
There was a problem hiding this comment.
We can name it mean_squared_error. Mentioning missing handling in the name seems a bit excessive. Better to leave this clarification to the documentation.
There was a problem hiding this comment.
If we have both mse from sklearn and our mean_squared_error it could be confusing.
We could remove sklearn's version, we haven't mentioned it in our public documentation (but have it in etna/metrics/__init__.py).
I'm also a bit of worried that by replacing sklearn's mse with our mse we changed list of available kwargs.
etna/metrics/base.py
Outdated
| @@ -128,7 +128,7 @@ class Metric(AbstractMetric, BaseMixin): | |||
| def __init__( | |||
| self, | |||
| metric_fn: MetricFunction, | |||
| mode: str = MetricAggregationMode.per_segment, | |||
| mode: str = MetricAggregationMode.per_segment.value, | |||
There was a problem hiding this comment.
We should consider setting here a plain string instead of an enum value
There was a problem hiding this comment.
You suggest using just "per-segment"? I suppose we should do this in all other places too?
Why do you think we should consider that?
etna/metrics/functional_metrics.py
Outdated
| @@ -41,6 +41,45 @@ def _get_axis_by_multioutput(multioutput: str) -> Optional[int]: | |||
| assert_never(multioutput_enum) | |||
|
|
|||
|
|
|||
| def mse_with_missing_handling(y_true: ArrayLike, y_pred: ArrayLike, multioutput: str = "joint") -> ArrayLike: | |||
There was a problem hiding this comment.
We can name it mean_squared_error. Mentioning missing handling in the name seems a bit excessive. Better to leave this clarification to the documentation.
etna/metrics/functional_metrics.py
Outdated
| `Wikipedia entry on the Mean squared error | ||
| <https://en.wikipedia.org/wiki/Mean_squared_error>`_ |
There was a problem hiding this comment.
I don't think it is a good idea to reference wikipedia. Better to replace with a more reputable source (e.g. Hyndman Forecasting) or to remove completely.
There was a problem hiding this comment.
Here I just repeated that we have done with mape and smape. I could replace link in those places too.
etna/metrics/functional_metrics.py
Outdated
| `Wikipedia entry on the Mean squared error | ||
| <https://en.wikipedia.org/wiki/Mean_squared_error>`_ | ||
|
|
||
| The nans are ignored during computation. |
There was a problem hiding this comment.
It would be helpful to also note what will be returned if all nans in the segment
Before submitting (must do checklist)
Proposed Changes
See #512
Closing issues
Closes #512.