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

VAR update #4666

Closed
wants to merge 2 commits into from
Closed

VAR update #4666

wants to merge 2 commits into from

Conversation

ckrapu
Copy link
Contributor

@ckrapu ckrapu commented Apr 24, 2021

This PR modifies the AR distribution to accommodate cross-series coefficients as desired in a vector autoregressive model. More information can be found at https://discourse.pymc.io/t/how-to-implement-vector-autoregression-with-interdependencies/4287/2 and #4665. This is marked as a WIP as I would like some advice on how to handle the constant argument for the AR distribution.

  • what are the (breaking) changes that this PR makes?
    Depending on the dimensions of rho, the logp function will either use an elementwise operation or a matrix multiplication for the independent and dependent time series cases, respectively. However, models which assume 2 or more dimensions indexing independent time series will no longer behave the same. In this case, the recommended approach is to reshape these two dimensions into a single one so that a rho tensor with shape [lags, d, d] can be interpreted as using multiple d x d cross-series coefficients.

  • important background, or details about the implementation
    There is a constant argument for the AR distribution which I am unable to decipher and have not been able to address the case in which constant is used.

  • are the changes—especially new features—covered by tests and docstrings?
    No new tests or doc modifications have been added yet.

  • linting/style checks have been run

  • consider adding/updating relevant example notebooks

  • right before it's ready to merge, mention the PR in the RELEASE-NOTES.md

@mjhajharia
Copy link
Member

from what i understand, only example notebooks and implementation details are left right?

@mjhajharia
Copy link
Member

are there existing notebooks that need to be updated or do we need to create new ones? @ckrapu i can take this up if you can tell me how far stuff is done and what would you have liked to do further :)

)
else:
bcast_fn = at.mul

if self.constant:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@almostmeenal The reason I paused on this PR was because I couldn't quite figure out exactly what the self.contant option was implementing. I essentially put in what looked to me like the broadcasted equivalent of it, but I'd like to figure it out some more.

@ckrapu
Copy link
Contributor Author

ckrapu commented Jun 7, 2021

Also, you are definitely welcome to take over this PR. For my part, I am going to try to dig up the reason for that formulation and I'll leave the answer here if I find it.

@michaelosthege michaelosthege marked this pull request as draft June 22, 2021 08:32
@michaelosthege michaelosthege changed the title [WIP] VAR update VAR update Jun 22, 2021
@mjhajharia
Copy link
Member

mentioning #5291, so we remember this PR once the new AR dist is made

@ricardoV94
Copy link
Member

Closing this as it went stale. Feel free to open a new PR on top of the newer V4 code ;)

@ricardoV94 ricardoV94 closed this Nov 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants