-
-
Notifications
You must be signed in to change notification settings - Fork 69
Multivariate Structural Statespace Components #529
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
base: main
Are you sure you want to change the base?
Multivariate Structural Statespace Components #529
Conversation
This is cool! I will review ASAP. Note that #450 is currently blocked by what I think is a pytensor bug |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is 🔥 @jessegrabowski 🤯
I just left a suggestion for what I think was a typo in the docstring. I'll merge once this is resolved, and then test all of this for our PyData tutorial -- probably this weekend.
Just a quick question: IIUC, now users can also have batched RegressionComponent
s, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is 🔥 @jessegrabowski 🤯
I just left a suggestion for what I think was a typo in the docstring.
Still missing this feature are:
-
Cycle
(currently worked on by @AlexAndorra) -
Seasonal
-
Regression
(currently worked on by @Dekermanjian)
We also need to:
- Make sure that there are tests that combined LevelTrend + AR + error for two observed variables with no interaction model matches two separate models for each, given the same parameters.
- Make sure that pytensor ops are used everywhere for building the SS matrices (no numpy/scipy)
I think I'm done for a first review from you on the |
2. Adjusted the regression component to allow multivariate regression component specification 3. Added a notebook for quick evaluation of the adjustments and additions made
2. replaced scipy block diag with pytensor block diag 3. Added forecast to test model in multivariate ssm notebook
Added multivariate regression-component
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AlexAndorra I left comments for you
Since it's my own PR I can't request changes. It's better in future if you fork the PR branch and open a new PR into this PR, then we can do the usual review workflow on your PR and merge it into this PR when we're ready
@AlexAndorra @Dekermanjian I want the names of parameters in the components to be really consistent and unsurprising. So please vote on:
Concrete examples for (3): For (4), I'm talking about the internal state names that will end up as labels for the R and Q matrices, nothing else. Also for default names, since all the are going to depend on the names in the multivariate case, should we:
|
Looks like we have consensus on To that end, let's also go with Regarding greek/descriptive, I'm really on the fence. I understand why @AlexAndorra is super anti-greek, and I've done implementations that specifically remove greek names (changing beta and gamma in BatchNorm to loc and scale here, for example). On the other hand, we do pay a cost when inventing our own names, unless they are already widely known/used in the specific literature. In the In actual fact, we already mix descriptive and greek names. In My suggestion is to punt on this and open a new issue to address package-wide naming conventions. For now, let's just make sure everything is of the form |
Ha ha, literally the reasoning behind my choice.
Totally agree, it's just that the way I see it, the cost is internal: even if the descriptive names are only our own (which is not the case here, IIUC):
And I like
Masterpiece, nothing to add, you killed me 🤣 |
2. added datetime multivariate forecast tests no exogenous variables 3. added test to check that parameter shapes and coordinate dims agree with one another
2. added all components to test_param_dims_coord
…jian/pymc-extras into multivariate-structural merge updates
…jian/pymc-extras into multivariate-structural merge updates
bug fix multivariate regression component univariate case
This is really close! I think basically we need to go through and make sure all the names in all the components follow the agreed format. Let's leave We just need a test for hidden state decomposition with multiple observed, and I think we're pretty much there. |
I added the test for multivariate decomposition. Basically it works, but it leaves a lot to be desired. I will open a separate issue to improve it. I think this is more or less done. @AlexAndorra and @Dekermanjian , if you could provide a final review then we can merge it. Also @Dekermanjian what do you want to do with your notebook that got merged in? We can either clean it up a bit to be a simple example, or we can remove it for now and you can do something more elaborate with missingness in the future. |
That is great! I will go through and review it today as soon as I am off from work.
I am not sure. I think that if the simple example doesn’t add anything on top of what Alex is working on then it would be best to remove it and do something more elaborate as you say with missingness. I am interested in figuring out how that will all work in a Bayesian State Space framework. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is looking great! The main issues I found in my review were related to the doctstring and certain components that still haven't adopted the schema of something_{self.name}
. These should be pretty quick to fix. We are almost there!
pymc_extras/statespace/models/structural/components/measurement_error.py
Outdated
Show resolved
Hide resolved
tests/statespace/models/structural/components/test_seasonality.py
Outdated
Show resolved
Hide resolved
tests/statespace/models/structural/components/test_seasonality.py
Outdated
Show resolved
Hide resolved
…hema and updated tests in accordance to naming changes
naming schema adherence
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe... this is it guys 🍾
This is looking fantastic! 👏 |
This PR lifts the requirement that models built with the structural sub-module of PyMC be univariate. It's a chonky PR, so I split it into commits. Most of the files changes are changed by the first commit, which is just reorganization of files. It is safe to ignore that one.
Here are the steps I followed:
Reorganize structural model modlue commit
Allow combination of component with different numbers of observed states
PR. I am confident this code can be improved.For now, we assume all states in a component follow the same parameterization. It's now also valid to add together the same component twice with different states to work around this (e.g.
AutoRegressive(order=1, observed_state_names=['data_1']) + Autoregressive(order=5, observed_state_names=['data_2'])
) would be a valid model with 2 observed states, but each has it's own autoregressive dynamics.When you pass a batch of observed_state_names, e.g.
LevelTrend(order=2, observed_state_names=['data_1', 'data_2'])
, the parameters will all be given a batch dimension, but will otherwise be the same as the base case.More docs coming, but I tried obsessively document what in there so far.
The logic for extending the components is pretty straight-forward -- mostly copying + block_diag or concat, but there are some corner cases that need attention.
This PR should be seen as a companion to #450. Instead of vectorizing across the computation of a model, we're concatenating models. There will be cases where this is superior -- for example when you want to explicitly model latent interactions between components. But in other cases, this approach will be worse. I am interested in having both.