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

Bug fixes for statespace #326

Merged
merged 9 commits into from
Apr 16, 2024
Merged

Conversation

jessegrabowski
Copy link
Member

@jessegrabowski jessegrabowski commented Apr 7, 2024

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

@jessegrabowski jessegrabowski added bug Something isn't working major labels Apr 7, 2024
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@jessegrabowski
Copy link
Member Author

jessegrabowski commented Apr 8, 2024

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

@ricardoV94
Copy link
Member

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

sure

@rklees
Copy link

rklees commented Apr 9, 2024

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
Copy link
Member

@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
Copy link

rklees commented Apr 9, 2024

I installed from #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
Copy link
Member Author

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
Copy link

rklees commented Apr 10, 2024 via email

@ricardoV94 ricardoV94 merged commit 0f03697 into pymc-devs:main Apr 16, 2024
8 checks passed
@jessegrabowski jessegrabowski deleted the bug-fixes branch June 1, 2024 05:28
@jessegrabowski jessegrabowski mentioned this pull request Jun 3, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working major
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New error message when building statespace_graph
3 participants