Conversation
- Restructured all Blackjax code to a new integrations file. - Updated `FilterBasedMCMC` class to utilize the new BlackJAX integration. - Added docstrings to MCMC methods and their configs
mattlevine22
left a comment
There was a problem hiding this comment.
Looks pretty solid.
- For PRs that create new API, we should probably include example usage in the docs:
- Add explicit usage example in the docstring so it appears in the docs
- It would also be "friendly" to show a chunk of numpyro demo-code that it can replace. For example, a code block where we call
NUTS/MCMCinsidewiththe numpyro way and get back a posterior...and then the same withFilterBasedMCMC. - Can we always expect the returned posterior to have the same attributes in both cases? That is, is there ever a time I can't use this
- How will it interact with upcoming
predict_times(i.e., supporting theFilter + Simulatorpattern)?
-
Since the purpose is "improved performance", I'd also suggest a deep dive that compares blackjax/numpyro (not exhaustively, but illustratively at least).
-
I'm a bit un-easy about redundant user-facing APIs (with Filter -> NUTS vs FilterMCMC(NUTS) ).
- Seems like we should have a SimulatorBasedMCMC or even
LatentDynamicsMCMC(type= Filter | Simulator | Smoother) - At that point, should we also have
LatentDynamicsSVI(type=Filter | Simulator | Smoother)?
- Why did the linter modify cd_dynamax and Cuthbert integrations?
- specifically it created/deleted some spaces in those files
- if this was a mistake, let's remove. otherwise, hopefully the linter stays happy going forward.
Agreed. Part of my inhibition here was how much of the existing API in the docs we'd want to replace (similar to (3) below).
Sure.
I'm not sure I follow on this specific point. This is mostly a simplification of using the interpretations directly (that I think should suffice for most users).
Yeah, I thought about that a bit... Seems reasonable, though I'd like to keep the corresponding aliases (
Maybe...
I think because the |
|
To clarify my concern about redundant API....I think we should have 1 recommended way of doing this and keep this consistent in the docs.
|
|
Will get around to this next week most likely, but to recall before I forget: the decision was:
|
|
Agreed. AND we should have a tutorial page specifically about the different legal ways to use 'with Filter' (you can build a conditioned model this way OR you can just wrap things as you go). |
Reruns notebooks accordingly
I can't get it to work...
|
I've updated the notebooks to move away from the There are probably a few documentation-related issues that remain from refactoring, but should be ready to review otherwise. I'd prefer to defer a more detailed notebook comparing different methods until we get a better empirical feel for things/figure out why NumPyro HMC had issues in #121. |
|
FYI I had to do |
In experimenting for #121, I found that Blackjax HMC has significantly better performance in some use cases for whatever reason (I suspect to do with CRN?). This PR adds support for a number of MCMC wrappers, including HMC, NUTS, and SGLD.