-
-
Notifications
You must be signed in to change notification settings - Fork 50
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
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
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 |
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>)] |
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. |
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. |
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 Android<https://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 #302<#302>. I'd need to see some code.
—
Reply to this email directly, view it on GitHub<#326 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AWKBIZJE357CJH6E7ACCUCLY4RHNFAVCNFSM6AAAAABF3ILJFSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBWGAYTQOBXG4>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
… scalar values, rather than 1d arrays
…ling `build_statespace_graph`
cff72e9
to
4e0fb96
Compare
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 givenshape=(1,)
ifdims
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 associateddims
). #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 whenstructural_model.build(verbose=True)
is called.In addition:
model.build_statespace_graph(observed_data)
. This is useful when setting exogenous data for anExogenousComponent
. In this case, a check is added that the index on the data passed to thebuild_statespace_graph
is the same as that found on any existingtime
dim.TimeSeasonality
previously always removed the first state because it is not identified. I added an option to keep it to I could test ifZeroSumNormal
could be used to identify all the states.Closes #318 , #297