Bug fixes for `statespace`

jessegrabowski commented 5 months ago

When I first wrote StructuralComponent, I had an idea that every model parameter should have dims associated with it. This had the consequence that even if a parameter should always be a scalar, such as standard deviations on shocks, it needed to be given shape=(1,) if dims were not used. This was a bit silly, so since #296, it has been my intention that scalar-only parameters should be scalars (and have no associated dims). #318 points out that the code is currently inconsistent -- some parameters are still having dims generated for them that don't need them, or vice-versa.

This PR should hopefully clean all this up. Parameters that are always scalars will never have dims associated with them, do not expect a shape argument, and will show shape=None in the model info output when structural_model.build(verbose=True) is called.

In addition:

I re-run the structural notebook to work after recent changes.
The name "time" can now be used as a dimension before calling model.build_statespace_graph(observed_data). This is useful when setting exogenous data for an ExogenousComponent. In this case, a check is added that the index on the data passed to the build_statespace_graph is the same as that found on any existing time dim.
TimeSeasonality previously always removed the first state because it is not identified. I added an option to keep it to I could test if ZeroSumNormal could be used to identify all the states.

Closes #318 , #297

review-notebook-app[bot] commented 5 months ago

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

jessegrabowski commented 5 months ago

Can we force merge this, the failures are unrelated (we need to update tests after https://github.com/pymc-devs/pymc/pull/7211). Also looks like the windows CI is totally borked by something in arviz.

ricardoV94 commented 5 months ago

Can we force merge this, the failures are unrelated (we need to update tests after https://github.com/pymc-devs/pymc/pull/7211). Also looks like the windows CI is totally borked by something in arviz.

sure

rklees commented 5 months ago

I downloaded the software and run your Structural Timeseries Modelling notebook. Get an error message in

ss_mod.build_statespace_graph(nile, mode="JAX")

saying

ValueError: All variables needed to compute inner-graph must be provided as inputs under strict=True. The inner-graph implicitly depends on the following shared variables [RandomGeneratorSharedVariable(<Generator(PCG64) at 0x165D86C00>)]

ricardoV94 commented 5 months ago

@rklees the code will only work with the latest version of PyMC after #329

You can downgrade to PyMC 5.12.0 in the meantime

rklees commented 5 months ago

I installed from https://github.com/pymc-devs/pymc-experimental/pull/326 and downgraded to pymc=5.12.0. After that, I could run the updated Structural Time Series modeling notebook and got the same results. However, running pymc-experimental on a synthetic datasets, which I am using already for months, provides results, which are completely different from those obtained with pymc-experimental version 0.0.16. Whereas version 0.0.16 provides very reasonable results, the 326 version provides unrealistic results. For instance, HDI intervals for the posterior predictive, are orders of magnitude too large.

jessegrabowski commented 5 months ago

If you think there's a bug I'm happy to track it down, but this PR shouldn't have touched anything related to that. If something was messed up my guess is that it would have been introduced in #302. I'd need to see some code.

rklees commented 5 months ago

I can send you two Jupyter notebooks, but dont know how to do that using github. If there is a simple way to do that, please let me know Roland

Sent from Outlook for Androidhttps://aka.ms/AAb9ysg

From: Jesse Grabowski @.> Sent: Tuesday, April 9, 2024 10:45:06 PM To: pymc-devs/pymc-experimental @.> Cc: Roland Klees @.>; Mention @.> Subject: Re: [pymc-devs/pymc-experimental] Bug fixes for statespace (PR #326)

If you think there's a bug I'm happy to track it down, but this PR shouldn't have touched anything related to that. If something was messed up my guess is that it would have been introduced in #302https://github.com/pymc-devs/pymc-experimental/pull/302. I'd need to see some code.

— Reply to this email directly, view it on GitHubhttps://github.com/pymc-devs/pymc-experimental/pull/326#issuecomment-2046018877, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AWKBIZJE357CJH6E7ACCUCLY4RHNFAVCNFSM6AAAAABF3ILJFSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBWGAYTQOBXG4. You are receiving this because you were mentioned.Message ID: @.***>

pymc-devs / pymc-experimental

Bug fixes for `statespace` #326