@mloning and @fkiraly the more I'm looking at this while looking to simplify the unit tests for these metrics, it makes more and more sense to me that the metric functions in _functions.py should also get updated to accept y_train via 1**kwargs` so we have a uniform interface.

Difference to the user passing info to functions will be relatively minimal.

Benefit is that y_train would be passed same way to function as the __call__ of the class version of the metric:

mean_absolute_scaled_error(y_true, y_pred, y_train=y_train, ... # Any other keyword args like multioutput)
mase = MeanAbsoluteScaledError()
mase(y_true, y_pred, y_train=y_train)

versus the current function setup accepting y_train as arg and class version of metric accepting it as keyword:

mean_absolute_scaled_error(y_true, y_pred, y_train, ... # Any other keyword args like multioutput)
mase = MeanAbsoluteScaledError()
mase(y_true, y_pred, y_train=y_train)

Note that I'll do whatever we decide for y_train for metrics that accept y_pred_benchmark.

I'll I've got this coded up but wanted to get your feedback on the approach before committing and pushing to Github.

@mloning and @fkiraly the more I'm looking at this while looking to simplify the unit tests for these metrics, it makes more and more sense to me that the metric functions in _functions.py should also get updated to accept y_train via 1**kwargs` so we have a uniform interface.

I've got this coded up but wanted to get your feedback on the approach before committing and pushing to Github.

Yes, very much agreed! I think this indeed makes unit tests easier, and any other exercise that requires a unified interface on the function level.

@fkiraly that sounds great. I think this should basically be ready for review. But I see the PR is failing the manylinux build. @mloning the details look like there is an issue with numba versioning in the linux builds, but not entirely sure what is going on there.

@mloning I think I've incorporated all your comments. As a bonus I've tweaked all the docstrings to better align with NumPy conventions and pass the pydocstyle checks.

I also opened an issue related to the creation of a BaseObject per our discussion above. I can start tackling that next.

@mloning I'm working on update to evaluate.

Ran into two minor hitches. First is that in the call to scoring within evaluate() the y_true and y_pred arguments were flipped (doesn't really matter for most metrics, but I fixed it).

The bigger hitch is that the test cases include in-sample predictions. In MeanAbsoluteScaledError() we have a check to make sure that y_train is before y_true. This makes sense in the context of forecast evaluation in general (and MASE's definition). But some of the test cases are looking at in-sample predictions and in those cases the check is failed in those cases; hence, the tests fail.

I can think of two options for approaching this:

1. We have separate test function for MASE that just tests the common configs with only the out-of-sample forecasting horizons or just pass on the tests with MASE when the forecasting horizon is negative
2. We remove the check in mean_absolute_scaled_error to require y_train prior to y_true

I am in the camp that MASE is designed to be applied when evaluating out-of-sample forecasts so we just include a line of code to exclude tests of evaluate for MASE when the forecasting horizon is negative (in-sample). That still ensures the MASE functionality is working in a way that makes sense for users (the whole evaluating in-sample forecasts is not a good gauge of the model's future predictive ability thing). But what do you think?

I could also figure out how to add a quick check in evaluate to raise an informative error if the user tries to use a metric that requires y_train with a CV object with a negative forecasting horizon (I'll also add note in docstring).

@RNKuhns I agree, but perhaps the check that y_train is prior to y_true is a bit over eager and we make everything a bit simpler by not enforcing that. In principle MASE still works for in-sample predictions, so perhaps we should exclude that option. This moves the responsibility to users for making sure they're evaluating models on genuine forecasts which I think is fine. But happy to follow your lead here @RNKuhns.

@RNKuhns, I'm with @mloning on this one - in my opinion, the metric should just model the "bare" mathematical/scientific object, not make any checks regarding a plausible use case. This is because user expectations - they will expect the code object to behave as the scientific one, and because of the domain driven design principle to follow this mapping.

A secondary point - in my opinion not the main argument here, but it's to the same effect - is that it can make sense for a metric in-principle to have training and test time points overlapping, for example when you are trying to estimate over-optimism of in-sample estimates. We shouldn't exclude a rare sensible use case.

Now merged - great work @RNKuhns 🎉