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] - Naive variance prediction estimator #1865

Merged
merged 41 commits into from
Mar 25, 2022

Conversation

IlyasMoutawwakil
Copy link
Contributor

@IlyasMoutawwakil IlyasMoutawwakil commented Jan 11, 2022

Reference Issues/PRs

#1687

What does this implement/fix? Explain your changes.

@fkiraly
As part of the probabilistic forecasting API, this will implement the _predict_var method to forecasters

What should a reviewer concentrate their feedback on?

STEP probabilistic forecasting API

PR checklist

  • add _predict_var and its corresponding tag to the forecasting extension template.
  • add NaiveVariance composite class which implements the 'simple strategy' for variance prediction.
  • implement _predict_var and its corresponding in NaiveVariance.
  • implement _predict_quantiles in NaiveVariance where quantiles are inferred from a normal distribution.
  • implement covariance prediction (cov=True)
For all contributions
  • I've added unit tests and made sure they pass locally.

@IlyasMoutawwakil
Copy link
Contributor Author

@fkiraly I have created the naive variance estimator and tested it with some forecasters but there some cases where it won't work:

  • if the forecaster can't do in sample forecasts on all of self._y.index, which can be solved (won't be able to compute variance for in sample forecasts)
  • if there's not enough training points (self._y.index[-1] < fh[-1])

Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

Good start!

Regarding the design:

  • you should also implement predict_var (without underscore) in the BaseForecaster. Boilerplate methods like check_fh should go there - see how the other predict_etc methods are implemented.
  • I think it´s better to return not a numpy.ndarray, but a pd.DataFrame (so the index is preserved)

I also left comments in the code.

Some bugs not mentioned there:

  • your code does not work if the forecaster is multivariate
  • I think your _predict_quantiles is incorrect
  • is your _predict_var implementing the formula we discussed? I think not? I also recommend to put the math in the docstring.

@fkiraly
Copy link
Collaborator

fkiraly commented Jan 20, 2022

if the forecaster can't do in sample forecasts on all of self._y.index, which can be solved (won't be able to compute variance for in sample forecasts)

I´m slightly confused - are your predictions not all out of sample, with a rolling window?

@fkiraly fkiraly added the module:forecasting forecasting module: forecasting, incl probabilistic and hierarchical forecasting label Jan 21, 2022
@IlyasMoutawwakil
Copy link
Contributor Author

I´m slightly confused - are your predictions not all out of sample, with a rolling window?

they are all out of sample now. I actually made it compute in sample as well so that it would be possible to compute in sample variance but I removed it now.

@IlyasMoutawwakil
Copy link
Contributor Author

Finally found why predict quantiles and intervals tests were failing. It was because I didn't use pd.MultiIndex to index quantiles and instead used plain indexes and columns

@fkiraly
Copy link
Collaborator

fkiraly commented Mar 25, 2022

I made some minor changes to the base class docstrings, extension template, and signature, to ensure everything is coherent after the batch of recent merges.

fkiraly
fkiraly previously approved these changes Mar 25, 2022
Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

Thanks for implementing!

Excellent, does not seem to require any additional changes imo.

I took the liberty of ensuring that this consistently merges with a number of other PR on probabilistic forecasting recently merged. Mostly docstring changes, but I also made sure that the signatures are consistent.

@fkiraly fkiraly changed the title DRAFT [ENH] - Naive variance prediction estimator [ENH] - Naive variance prediction estimator Mar 25, 2022
@fkiraly fkiraly merged commit 4c32797 into sktime:main Mar 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module:forecasting forecasting module: forecasting, incl probabilistic and hierarchical forecasting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants