-
-
Notifications
You must be signed in to change notification settings - Fork 59
Add utility to convert Model to and from FunctionGraph #111
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
Conversation
ricardoV94
commented
Feb 20, 2023
•
edited
Loading
edited
ba7968b
to
5e1c8b8
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.
@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.
5e1c8b8
to
08f4a72
Compare
Ready for review! |
2473bfe
to
05c9c24
Compare
d3a1b61
to
103025b
Compare
07c61a7
to
5ce8973
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.
This looks really great @ricardoV94. I’ve got two comments:
- 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.
- 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_
andy__
make it seem that way to me
Mutable dimensions should be working via the There's a test here: https://github.com/pymc-devs/pymc-experimental/blob/5ce89730b419a96a3ce4cc5dc823b7ca2f0446ac/pymc_experimental/tests/utils/test_model_fgraph.py#L69 The only missing cases I could think were initvals and nested models, both of which raise explicitly in the conversion function.
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. |
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.
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