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

Add utility to convert Model to and from FunctionGraph #111

Merged
merged 2 commits into from
May 22, 2023

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Feb 20, 2023

import pymc as pm
from pytensor import dprint
from pymc_experimental.utils.model_fgraph import fgraph_from_model, model_from_fgraph

with pm.Model (coords={"trial": range(3)}) as m:
    y_obs = pm.MutableData("y_obs", [1., 2., 3.], dims=("trial",))
    mu = pm.Normal("mu")
    sigma = pm.HalfNormal("sigma")
    y = pm.Normal("y", mu, sigma, observed=y_obs, dims=("trial",))

pm.model_to_graphviz(m)

fg = fgraph_from_model(m)
dprint(fg)

... # Manipulate model

new_m = model_from_fgraph(fg)
pm.model_to_graphviz(new_m)

@ricardoV94 ricardoV94 changed the title Implement utility to convert PyMC modelt to and from FunctionGraph Implement utility to convert PyMC model to and from FunctionGraph Feb 20, 2023
@ricardoV94 ricardoV94 changed the title Implement utility to convert PyMC model to and from FunctionGraph Utility to convert PyMC model to and from FunctionGraph Feb 20, 2023
@ricardoV94 ricardoV94 added the enhancements New feature or request label Feb 20, 2023
Copy link
Contributor

@lucianopaz lucianopaz left a comment

Choose a reason for hiding this comment

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

@ricardoV94, thanks for being able to kick this off. I have a bunch of comments, but I feel that most of the design decisions should be discussed elsewhere. I've opened this discussion to track the design stuff. My main critique is that the new Op that you introduced will make it harder to apply rewrites, because they will have to weigh what to do about the FreeRV or ObservedRV or Deterministic wrapping Op. I think that we can continue that discussion in the thread that I linked.

pymc_experimental/utils/model_fgraph.py Show resolved Hide resolved
pymc_experimental/utils/model_fgraph.py Outdated Show resolved Hide resolved
pymc_experimental/utils/model_fgraph.py Outdated Show resolved Hide resolved
@ricardoV94 ricardoV94 requested a review from ferrine March 29, 2023 10:17
@ricardoV94 ricardoV94 marked this pull request as ready for review April 12, 2023 17:16
@ricardoV94
Copy link
Member Author

Ready for review!

@ricardoV94 ricardoV94 changed the title Utility to convert PyMC model to and from FunctionGraph Add utility to convert PyMC model to and from FunctionGraph Apr 12, 2023
@ricardoV94 ricardoV94 changed the title Add utility to convert PyMC model to and from FunctionGraph Add utility to convert Model to and from FunctionGraph Apr 14, 2023
@ricardoV94 ricardoV94 force-pushed the fgraph_model branch 3 times, most recently from 2473bfe to 05c9c24 Compare April 14, 2023 11:43
@ricardoV94 ricardoV94 force-pushed the fgraph_model branch 2 times, most recently from d3a1b61 to 103025b Compare April 20, 2023 12:42
@ricardoV94 ricardoV94 force-pushed the fgraph_model branch 2 times, most recently from 07c61a7 to 5ce8973 Compare May 4, 2023 10:26
Copy link
Contributor

@lucianopaz lucianopaz left a comment

Choose a reason for hiding this comment

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

This looks really great @ricardoV94. I’ve got two comments:

  1. This is a small nit, but could you add a todo list with the things that still aren’t covered by the transformation to fgraph. At the moment I can think of the initval, mutable dimensions, but there must be some more.
  2. I can’t wrap my head around the special care you take with deterministics. It looks like the rebuilt model might have a different structure of the deterministics than the original one? At least the test that you wrote with y_ and y__ make it seem that way to me

@ricardoV94
Copy link
Member Author

ricardoV94 commented May 10, 2023

This is a small nit, but could you add a todo list with the things that still aren’t covered by the transformation to fgraph. At the moment I can think of the initval, mutable dimensions, but there must be some more.

Mutable dimensions should be working via the NamedVars.

There's a test here:

The only missing cases I could think were initvals and nested models, both of which raise explicitly in the conversion function.

I can’t wrap my head around the special care you take with deterministics. It looks like the rebuilt model might have a different structure of the deterministics than the original one? At least the test that you wrote with y_ and y__ make it seem that way to me

Yes the rebuilt model will never have the deterministics "floating outside" of the graph (unless they are a direct view on another model variable), so they show up nicely in graphviz.

This may be different than the original user model, but I don't think it's something we have to worry about preserving.

On the other hand, the fgraph model will never have deterministics in the middle of the graph, they will always be "floating outside". This way they shouldn't interfere with rewrites.

Copy link
Contributor

@lucianopaz lucianopaz left a comment

Choose a reason for hiding this comment

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

I think this looks well enough to merge. Let’s try it out on the actual applications to see if we need to adapt anything

@ricardoV94 ricardoV94 merged commit bb2359b into pymc-devs:main May 22, 2023
7 checks passed
@ricardoV94 ricardoV94 deleted the fgraph_model branch July 25, 2023 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancements New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants