-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Start adding GRW RV Op and distribution #5298
Conversation
Code inside the Anyway the idea with aeppl is that we don't need to build a new RandomVariable for every possible distribution, but use Aesara directly (as you are doing with that scan) to generate random draws. Similarly, instead of needing a custom logp method for every distribution we use aeppl to generate the logp graph, given the corresponding Aesara random generating graph. The thing that is needed is to make |
Also to illustrate the point of this approach, the only thing that is needed to create any other random walk is to change the distribution used in that scan graph rv, updates = aesara.scan(
fn=lambda prev_value: pm.StudentT.dist(prev_value + mu, sigma, nu),
outputs_info=np.array(0.0),
n_steps=size,
)
# ignoring: updates have to be managed differently when using RandomVariables via `.dist` Instead of having to create them one by one we can offer all at once in a my_srw = pm.RandomWalk("my_srw", dist=pm.StudentT.dist(mu, sigma, nu), size=10)
|
These are great pointers thank you! |
@ricardoV94, reviewed the other prs and thought through this some. Do you mind if I learn how to implement a "fixed" GRW RV first in this PR, get that working, then follow up with a RW RV generalization? Thatll help me break up unknown bits that I need to learn into smaller unknown bits for me |
If you want to take it in small steps I think limiting yourself to the case of Gaussian innovations might help. I would still suggest you sidestep writing the RandomVariable altogether and try to use the WIP Otherwise you'll just be practicing writing traditional PyMC distributions. That can be useful for temporary backwards compatibility of V4, but it will have a smaller impact than understanding how to make use of aeppl's flexibility. |
Great thanks for this advice, I'll start working off of that branch |
Codecov Report
@@ Coverage Diff @@
## main #5298 +/- ##
==========================================
+ Coverage 88.58% 88.82% +0.23%
==========================================
Files 75 75
Lines 13716 13736 +20
==========================================
+ Hits 12151 12201 +50
+ Misses 1565 1535 -30
|
@ricardoV94 Let me know if you like this GRW RV implementation. Per our conversation for now we'll
Next steps for me to to create the GRW Distribution in old style After thats working I'll work on
|
Are you planning to do this all in one PR or separate ones? |
Separate ones ideally i updated the comment to make it more clear what my plan is, let me know if you think its a reasonable course of action |
Are you planning to refactor the PyMC distribution to work with the explicit RV from this PR? |
5d097b1
to
2f71b49
Compare
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.
small tweak to comments
There's some indentation error going on |
Looks like the init dist is causing some test failure. I can look into this weekend, next 72 hours or so https://github.com/pymc-devs/pymc/runs/5788430816?check_suite_focus=true#step:7:411 |
Alright well this passes except black which I think is broken, and mypy which Im pretty sure is broken, and flaky codecov. Should I just merge and those get fixed up in other prs? |
Co-authored-by: Ricardo Vieira <28983449+ricardoV94@users.noreply.github.com>
Co-authored-by: Ricardo Vieira <28983449+ricardoV94@users.noreply.github.com>
Co-authored-by: Ricardo Vieira <28983449+ricardoV94@users.noreply.github.com>
Co-authored-by: Ricardo Vieira <28983449+ricardoV94@users.noreply.github.com>
Starting to add time series support to v4 on step at a time (See what I did there?)
Scope of this PR is just to add the random variable, not the distribution as this might take me a while and I'll likely have a lot of basic questions
#5290