New forecasting metrics by RNKuhns · Pull Request #801 · sktime/sktime

RNKuhns · 2021-04-12T13:24:54Z

Reference Issues/PRs

Fixes #671. See discussion on closed PR #672 that this pull request will complete.

What does this implement/fix? Explain your changes.

Adds additional forecasting metrics as discussed in #671.

This is a draft pull request. I need to finish the docstrings (I'm adding examples to docstrings) and also write unit tests but wanted to give you a heads up of the functionality.

The pull request adds new forecasting performance metrics (see list below) that can each work with multivariate series (but default to work with univariate forecasts). Also changes naming conventions to align with scikit-learn (e.g. mean_absoluate_error rather than mae or mae_loss).

The end result would be the following performance metric functions (each with a corresponding class version for scoring):

relative_error,
mean_asymmetric_error,
mean_absolute_scaled_error,
median_absolute_scaled_error,
mean_squared_scaled_error,
median_squared_scaled_error,
mean_absolute_error,
mean_squared_error,
median_absolute_error,
median_squared_error,
mean_absolute_percentage_error,
median_absolute_percentage_error,
mean_squared_percentage_error,
median_squared_percentage_error,
mean_relative_absolute_error,
median_relative_absolute_error,
geometric_mean_relative_absolute_error,
geometric_mean_relative_squared_error

Does your contribution introduce a new dependency? If yes, which one?

None. Pinning Scikit-learn dependency to >= 0.24.* was done separately.

What should a reviewer concentrate their feedback on?

Focus on changes in ./performance_metrics/forecasting/_functions.py and ./performance_metrics/forecasting/_classes.py.

Finishing fixes elsewhere in module where old performance metric names (e.g. sMAPE) are imported.

Any other comments?

PR checklist

For all contributions

I've added myself to the list of contributors.
Optionally, I've updated sktime's CODEOWNERS to receive notifications about future changes to these files.
I've added unit tests and made sure they pass locally.

For new estimators

I've added the estimator to the online documentation.
I've updated the existing example notebooks or provided a new one to showcase how my estimator works.

Other minor tweaks to forecasting functions that were identified as part of unit testign.

Fixed __init__ in all classes.

Verification applies to relative error functions.

Since naming convention of forecasting performance metrics was changed, the old references to things like sMAPE and smape_loss had to be updated.

Added examples to docstrings. Fixed squared percentage error functions to match formula in sources. Also fixed geometric_mean_relative_squared_error and geometric_mean_relative_absolute_error

MixIn

Fixed merge after conversion of master branch to main branch

fkiraly · 2021-04-22T15:18:02Z

Having classes and functions comes from scikit-learn I think, where all performance metrics are principally functions, but you wrap them in a scorer when passing them to higher-level functionality like model evaluation or tuning.

I do agree with that, @mloning, but I feel they are not homogenously exposed - which contributes to interface inhomogenity.

Having said that, MetricFunctionWrapper should be private and only used internally for constructing classes.

That's indeed an instance of what I mean above.
Template metrics can be public, but factories should probably be private. There should also be public classes corresponding to concrete metrics - for every function as well, no?

In terms of software design, even if we only expose classes, it still makes sense to encapsulate performance metrics in functions I think, and then wrap them in classes to attach additional information like greater_is_better. This seems to be the common pattern in other libraries like pytorch too.

Also agreed, but properties shouldn't be constructor parameters (it's not something you can choose/set, but an intrinsic property - a static class variable).

mloning · 2021-04-22T17:05:45Z

I do agree with that, @mloning, but I feel they are not homogenously exposed - which contributes to interface inhomogenity.

Okay, what changes to do you suggest to make the interface for classes and functions more consistent? Should we do them in this PR or a follow-up PR?

There should also be public classes corresponding to concrete metrics - for every function as well, no?

In principle, yes. Not sure if there is a class for every function already @RNKuhns?

Also agreed, but properties shouldn't be constructor parameters (it's not something you can choose/set, but an intrinsic property - a static class variable).

I see, I suggest to make the factory methods private for now and change that in a future PR.

RelativeLoss metric class.

…sktime into new-forecasting-metrics

class

mloning

One last change from my side!

sktime/performance_metrics/forecasting/_classes.py

…sktime into new-forecasting-metrics

mloning

Looks good to be merged to me!

@fkiraly let me know if you want to take another look before we merge.

sktime/performance_metrics/forecasting/_classes.py

fkiraly · 2021-04-26T10:28:54Z

Nice!

I have a few comments:

the non-optional call signature of the functions is inconsistent and violates the "uniformity" pattern - some functions require y_pred_benchmark. I would recommend to let all functions to accept this as an argument (e.g., by using a kwargs catch argument), and let the metrics that don't need it simply ignore it when passed (instead of producing an error).
I would strongly suggest to loop multioutput and sp parameters through to the class constructors - otherwise you can't use classes with multi-output metrics; perhaps even horizon_weight (though I'm not sure about that one)
I would suggest that classes inherit from sklearn BaseEstimator, otherwise you can't tune parameters in metrics, e.g., when using regularization or penalty parameters in the metric to tune for the same, unregularized/unpenalized metric as a generalization criterion

Minor issues:

strange capitalization of mixin (should be Mixin not MixIn)
sp appears in docstrings but not in constructor

RNKuhns · 2021-04-26T12:33:43Z

@fkiraly thanks for the feedback. I've made most of the changes you've discussed.

Good catch on sp in docstrings and not constructor. I've got that updated in the 4 scaled metrics that use it.
I'm updating the mixin capitalization now (I was thinking 2 words like mix in are MixIn per CapWords Python class naming conventions, but I see that Scikit-learn uses Mixin).
_MetricFunctionWrapper is already setup to inherit from BaseEstimator. The other metrics inherit from _MetricFunctionWrapper, so they should all have its attributes/methods built-in.

I haven't made the update with y_pred_benchmark because Markus and I are planning to discuss how to deal with y_train and y_pred_benchmark as a follow-on PR (see #712, I believe we noted y_pred_benchmark in the discussion even though the title just mentions y_train).

We could also include the potential inclusion of multioutput and horizon_weight in the PR related to #712; however, I'm open to adding them to each functions constructor since they are parameters of all the underlying functions. It would be really quick to do that (but for the time being we aren't actually using that functionality so that could also be a future PR to add it). @fkiraly and @mloning let me know what you want me to do on those.

mloning

I'd prefer to merge this now and add additional functionality in separate PRs.

I'm not sure if all functions should accept y_train and y_pred_benchmark, but let's discuss this in a separate issue/PR.

mloning · 2021-04-29T18:21:07Z

@fkiraly any more comments? Otherwise I'll merge this tomorrow :)

mloning · 2021-04-30T14:02:58Z

Now merged @RNKuhns - thanks again 🎉 👍

jerronl · 2021-04-30T20:45:44Z

from sktime.performance_metrics.forecasting import smape_loss

ImportError: cannot import name 'smape_loss' from 'sktime.performance_metrics.forecasting' (/usr/local/lib/python3.6/dist-packages/sktime/sktime/performance_metrics/forecasting/init.py)

mloning · 2021-04-30T23:11:51Z

@jerronl smape_loss was renamed to mean_absolute_percentage_error

mloning · 2021-04-30T23:12:53Z

Perhaps we should add deprecation warnings for the old loss functions?

RNKuhns and others added 30 commits December 26, 2020 11:30

Changes to pass pre-commit workflow

aecc0a7

Merge branch 'master' of https://github.com/alan-turing-institute/sktime

3680e24

Added unit tests and reworked new functions

a195474

Added name to contributorsrc and minor fixes

ad31048

Updated instlalation.rst Windows section

3f055fc

Merge remote-tracking branch 'upstream/master'

6449f26

New functions in performance_metrics.forecasting

dc83b19

Added root median squared scaled error.

adc7013

Other minor tweaks to forecasting functions that were identified as part of unit testign.

Added RootMedianSquaredScaledError.

58991d7

Fixed __init__ in all classes.

Removed RelativeLoss and MeanAsymmetricError

df8d20d

Add root_median_squared_scaled_error to __all__

cdd2da4

Added root_median_squared_scaled_error to __all__

da92a34

Changed y_test to y_true in check_y_true_pred

b9c1f96

Added validation to y_pred_benchmarks.

fce95d7

Verification applies to relative error functions.

Updated tests of new forecasting metrcs

6654db9

Updated module references new performance metrics

2dd2080

Since naming convention of forecasting performance metrics was changed, the old references to things like sMAPE and smape_loss had to be updated.

Updated to reference new forecasting metrics

94d1bb7

Merge remote-tracking branch 'upstream/master'

7bf3492

Moved check_horizon_weights to utils.validation

0e465dc

Updated docstrings and fixed bugs in functions

7c8d38a

Added examples to docstrings. Fixed squared percentage error functions to match formula in sources. Also fixed geometric_mean_relative_squared_error and geometric_mean_relative_absolute_error

Merge branch 'master' into master

4078dba

Removed validations used in forecasting metrics

f07a95e

Streamlined loss functions and classes

3ae1887

Use sklearn to validate y_train/y_pred benchmark

c99392a

updated mean_asymmetric_error docstring

424acc8

Added MeanAsymmetricError class and corresponding

0e940ff

MixIn

Updated based on performance metric changes

885242c

Updated unit tests based on changes to metrics

1d1e6d6

Merge branch 'master' of https://github.com/RNKuhns/sktime

c1453c4

Merge remote-tracking branch 'upstream/main' into main

61fbde2

Fixed merge after conversion of master branch to main branch

RNKuhns added 5 commits April 23, 2021 13:45

Made factory classes non-public. Added

0a3d3ee

RelativeLoss metric class.

Merge branch 'new-forecasting-metrics' of https://github.com/RNKuhns/…

8c554e2

…sktime into new-forecasting-metrics

Added asymmetric error and relative loss metrics

4b1c51d

Bug fix in relative_loss func and RelativeLoss

73bce7c

class

Added asymmetric error and relative loss metrics

92f6a7e

RNKuhns dismissed mloning’s stale review via 92f6a7e April 23, 2021 18:44

RNKuhns and others added 2 commits April 23, 2021 14:47

Added missing metrics to docs

7e06a7b

Merge branch 'main' into new-forecasting-metrics

22b4509

mloning reviewed Apr 23, 2021

View reviewed changes

sktime/performance_metrics/forecasting/_classes.py Outdated Show resolved Hide resolved

RNKuhns added 2 commits April 24, 2021 12:15

Removed use of greater_is_better in metric call

8d38793

Merge branch 'new-forecasting-metrics' of https://github.com/RNKuhns/…

2ff875b

…sktime into new-forecasting-metrics

RNKuhns mentioned this pull request Apr 24, 2021

More evaluation metrics added #836

Closed

mloning previously approved these changes Apr 24, 2021

View reviewed changes

SinghShreya05 mentioned this pull request Apr 25, 2021

Added two forecasting metrics #837

Closed

fkiraly reviewed Apr 26, 2021

View reviewed changes

sktime/performance_metrics/forecasting/_classes.py Outdated Show resolved Hide resolved

fkiraly reviewed Apr 26, 2021

View reviewed changes

sktime/performance_metrics/forecasting/_classes.py Outdated Show resolved Hide resolved

RNKuhns added 2 commits April 26, 2021 08:10

Fixed MixIn typo as Mixin

2958677

Fixed missing sp parameter in scaled metrics

7b7331d

RNKuhns dismissed mloning’s stale review via 7b7331d April 26, 2021 12:30

mloning approved these changes Apr 26, 2021

View reviewed changes

RNKuhns mentioned this pull request May 4, 2021

Uniform handling of y_train in forecasting performance metrics #712

Closed

Uh oh!

Conversation

RNKuhns commented Apr 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Does your contribution introduce a new dependency? If yes, which one?

What should a reviewer concentrate their feedback on?

Any other comments?

PR checklist

For all contributions

For new estimators

Uh oh!

fkiraly commented Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mloning commented Apr 22, 2021

Uh oh!

mloning left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mloning left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fkiraly commented Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RNKuhns commented Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mloning left a comment

Choose a reason for hiding this comment

Uh oh!

mloning commented Apr 29, 2021

Uh oh!

mloning commented Apr 30, 2021

Uh oh!

jerronl commented Apr 30, 2021

Uh oh!

mloning commented Apr 30, 2021

Uh oh!

mloning commented Apr 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

RNKuhns commented Apr 12, 2021 •

edited

Loading

fkiraly commented Apr 22, 2021 •

edited

Loading

fkiraly commented Apr 26, 2021 •

edited

Loading

RNKuhns commented Apr 26, 2021 •

edited

Loading