& ris-bali [ENH] statsmodels DynamicFactor interface

[lbventura]

Reference Issues/PRs

Addresses #2349. There are multiple failed tests due to the behavior of the _predict method, but further exploration in testfile.ipynb suggests that the method's behavior is actually correct. Could be that tests are wrong?

What does this implement/fix? Explain your changes.

Deeper look into #2349 failed tests using check_estimator.

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

What should a reviewer concentrate their feedback on?

Any other comments?

(At least) Two outstanding points:

1. Implement the predict_proba method;
2. Create an example notebook - one cool example would be to forecast economic variables;

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.
• The PR title starts with either [ENH], [MNT], [DOC], or [BUG] indicating whether the PR topic is related to enhancement, maintenance, documentation, or bug.

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.

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[fkiraly]

I think I understand the problem now
the test is fine, it intentionally checks for indices that do not start at zero
the problem is with the (inner, statsmodels) dynamic factor model, it discards indices and produces forecasts starting at len(y) rather than at y.index[-1]
the solution would be to manually correct for that and simply overwrite the indices of what comes out of the model

[lbventura]

Weirdly pre-commit is failing in "Install and test / code-quality (pull_request)" due to flake8, but worked in my local machine? See the latter below

[fkiraly]

You probably don't have all the linting checks installed locally then?
You can always go on "details" here to see what's wrong:

(I know what's wrong, and it is easy to fix - but kindly have a look yourself 😄 )

[fkiraly]

Looks good!

Blocking comments:

• we need to remove the notebook testfile.ipynb.
• the estimator should be added to the API reference in an appropriate section.

I also left a number of small, non-blocking comments, above.

[lbventura]

the estimator should be added to the API reference in an appropriate section.

Looks good!

Blocking comments:

* we need to remove the notebook `testfile.ipynb`.

* the estimator should be added to the API reference in an appropriate section.

I also left a number of small, non-blocking comments, above.

On the second point, what exactly should be done? One example would be helpful :)

[fkiraly]

On the second point, what exactly should be done? One example would be helpful :)

goto docs.source.api_reference.forecasting and add an entry, following the pattern.
If not sure, jump quickly on discord.

[fkiraly]

the right section, is that perhaps "Structural time series models"?

[lbventura]

the right section, is that perhaps "Structural time series models"?

Only saw this now but yes, it is where I placed it.

[fkiraly]

Only saw this now but yes, it is where I placed it.

Well, you managed to do it incorrectly, as the failed doctest tells us. Let me check...

[fkiraly]

... odd, looks correct.

[fkiraly]

ah, looks like a typo in the "references" section of the new estimator. See "details" next to the doc build, maybe you can find out what exactly the typo is.

[lbventura]

... odd, looks correct.

It is complaining that the output is too long? Could it be due to the fact that I changed full_output to True?

[fkiraly]

hm, estimator tests are failing now - any idea why? Perhaps you would like to change it back to the last state where they were not?

[lbventura]

hm, estimator tests are failing now - any idea why? Perhaps you would like to change it back to the last state where they were not?

The only reasons I can think of are:

1. Having changed the sciptype from multivariate to both. I reverted back to multivariate;
2. Having changed the outputs of the model (the full_output point discussed above).

Let us see if the new commit addresses the problem.

[fkiraly]

The only reasons I can think of are:

1. Having changed the sciptype from multivariate to both. I reverted back to multivariate;

But, should it not be both? This estimator can deal with univariate data, can it not?

2. Having changed the outputs of the model (the full_output point discussed above).

Why did you do this?

The docs are still not building, btw, kindly check for typos.

[lbventura]

The only reasons I can think of are:

1. Having changed the sciptype from multivariate to both. I reverted back to multivariate;

But, should it not be both? This estimator can deal with univariate data, can it not?

This was just a quick edit to see whether the problem comes from here. EDIT: it does, once one changes the scitype to "both", there are several tests in check_estimator which fail. See a few examples below:

I think it should also work for univariate, but I will check. EDIT: it also does not work in statsmodels. y is just a 1D series generated with _make_series.

2. Having changed the outputs of the model (the full_output point discussed above).

Why did you do this?

The docs were being built fine in a previous commit, reverted to see if this was the change that broke it, but aparently not.

The docs are still not building, btw, kindly check for typos.

Will do

[fkiraly]

I think it should also work for univariate, but I will check. EDIT: it also does not work in statsmodels. y is just a 1D series generated with _make_series.

Ah, ok - then the value "multivariate" that you originally had was correct.

[lbventura]

Tried to replicate arima.py forecaster docstring, hopefully this solves the readthedocs problem. Unfortunately the build logs are not very clear.

[fkiraly]

hopefully this solves the readthedocs problem. Unfortunately the build logs are not very clear.

Yes, looks like it! All green!

[fkiraly]

Nice, thanks!

[lbventura]

Nice, thanks!

Great, on to the next topic (which should be implementing predict_proba if I'm not lazy)!