Make basic INLA interface and simple marginalisation routine #533
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
| else: | ||
| dependent_rvs_dim_connections = [ | ||
| (None,), | ||
| ] |
There was a problem hiding this comment.
I believe this is redundant in INLA and is only necessary for Discrete marginal stuff (and thus it is fine to define this as None). Please correct me if I'm wrong.
There was a problem hiding this comment.
Yes I think in the refactor WIP PR the base marginalRV class doesn't have this property
There was a problem hiding this comment.
In lines 613-619, marginalization_op is defined consistently regardless of the MarginalRV it's built from. For compactness, I think it's fine to set it to None through this else block rather than having a seperate marginalize_constructor line?
marginalization_op = marginalize_constructor(
inputs=inner_inputs,
outputs=inner_outputs,
dims_connections=dependent_rvs_dim_connections,
dims=dims,
**marginalize_kwargs,
)
| d = 3 # 10000 # TODO pull this from x.shape (or similar) somehow | ||
| rng = np.random.default_rng(12345) | ||
| x0 = pytensor.graph.replace.graph_replace(x0, {marginalized_vv: rng.random(d)}) |
There was a problem hiding this comment.
Not within the scope of this PR, but is there a nice (i.e. pytensor-native) way to specify an RNG vector of shape (d,) without np.random? I believe that rng.random(d) crashes when we set d = marginalized_vv.shape[0] because marginalized_vv is a pt tensor, so it's unhappy with d being something symbolic rather than an int. For now I've worked around it by hardcoding d.
There was a problem hiding this comment.
If it's a one-liner fix, I'll include that in this PR of course.
There was a problem hiding this comment.
pt.random.uniform(size=x0.shape)
There was a problem hiding this comment.
Throws the following:
ValueError: Random variables detected in the logp graph: {uniform_rv{"(),()->()"}.out, uniform_rv{"(),()->()"}.out}.
This can happen when mixing variables from different models, or when CustomDist logp or Interval transform functions reference nonlocal variables.
There was a problem hiding this comment.
That's correct the logp is now technically a stochastic function. We can allow it to be as is but perhaps better not to add randomness? Can we have a reasonable deterministic starting point?
| else: | ||
| dependent_rvs_dim_connections = [ | ||
| (None,), | ||
| ] |
There was a problem hiding this comment.
Yes I think in the refactor WIP PR the base marginalRV class doesn't have this property
398979a to
c6010f3
Compare
dad163c to
a473e87
Compare
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
| # Check if latent field is Gaussian | ||
| if not isinstance(x.owner.op, MvNormal): | ||
| raise ValueError( | ||
| f"Latent field {x} is not instance of MvNormal. Has distribution {x.owner.op}." | ||
| ) |
There was a problem hiding this comment.
Should we relax this to a warning @theorashid?
Hey @Michal-Novomestsky, thanks for all your work here! I recently returned from travel so I'm just now catching up on my backlog. I assume you're asking why the Read the Docs workflow is failing? Opening the CI logs and expanding the line with the error displays the following stack trace:
Is this helpful? |
@maresb Ah yep that makes sense - this PR is dependent on two other PRs in PyMC and Pytensor respectively, so it (and the rest of the unittests) are failing because those haven't been merged yet. Thanks for the help! |
Addresses #532 and #344.
Relies on pymc-devs/pytensor#1582 and pymc-devs/pymc#7895.
Currently uses a closed-form solution for a specific case (nested normals) while awaiting pymc-devs/pytensor#1550