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

Mark pm.Data nodes in graphviz graphs. #3491

Merged
merged 1 commit into from
May 31, 2019

Conversation

rpgoldman
Copy link
Contributor

No description provided.

@rpgoldman
Copy link
Contributor Author

Need to revise a test to make this OK.
Also, I would be interested in a review. My means of marking the theano Tensor as being a Data object is kludgy, and perhaps someone has a better idea.

@rpgoldman
Copy link
Contributor Author

rpgoldman commented May 23, 2019

Here's an example of the proposed kind of diagram:

image

@@ -108,7 +104,15 @@ def _make_node(self, var_name, graph):
if hasattr(v, 'distribution'):
distribution = v.distribution.__class__.__name__
else:
distribution = 'Deterministic'
is_data = False # type: bool
Copy link
Contributor

Choose a reason for hiding this comment

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

From looking at the source code for the Data class, you only need to test if v is an instance of SharedVariable. A deterministic will never be an instance of SharedVariable because of the call to copy which at most will return a view of a shared variable, which is not an instance of SharedVariable itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lucianopaz
Thanks! Fixed that and the test that was failing. If this is OK with people, we can merge it (although I will clean up the history first).
I would like to figure out how to put data that are observations below the observed node, instead of above, but I don't sufficiently understand what can be in the observed property of a random variable. Do I need to check to see if it's a SharedVariable, and then see if it has a name property? Are those sufficient and necessary conditions?

@rpgoldman
Copy link
Contributor Author

With the latest commit, we now get graphs like this -- note the new placement of obs_gfp node:
image

@ColCarroll
Copy link
Member

Thanks for including samples -- do the Data objects know what shape they are? I guess that's funny for a shared variable, but I find it very useful for debugging if this data is printed in the node.

@rpgoldman
Copy link
Contributor Author

Thanks for including samples -- do the Data objects know what shape they are? I guess that's funny for a shared variable, but I find it very useful for debugging if this data is printed in the node.

I didn't look into this that carefully. It looks like the observation ones do, at least to the extent of being placed in the right plates (all those observations in my model have the same shape -- they are vectors 1043 long).

1 similar comment
@rpgoldman
Copy link
Contributor Author

Thanks for including samples -- do the Data objects know what shape they are? I guess that's funny for a shared variable, but I find it very useful for debugging if this data is printed in the node.

I didn't look into this that carefully. It looks like the observation ones do, at least to the extent of being placed in the right plates (all those observations in my model have the same shape -- they are vectors 1043 long).

@rpgoldman
Copy link
Contributor Author

Just pushed a cleaned-up version with all the various blind-alleys, etc. removed from the history. If people are happy with it after that, let's merge. I don't know if it needs to be written up in the release notes, since it's a pretty straightforward cosmetic enhancement.

@rpgoldman
Copy link
Contributor Author

Sorry to nag, but -- ok to merge?

@rpgoldman rpgoldman changed the title [WIP] Mark pm.Data nodes in graphviz graphs. Mark pm.Data nodes in graphviz graphs. May 27, 2019
Copy link
Member

@ColCarroll ColCarroll left a comment

Choose a reason for hiding this comment

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

Looks good to merge if you can confirm that not every shape is a rectangle in the resulting graph!

pymc3/model_graph.py Show resolved Hide resolved
else:
distribution = 'Deterministic'
attrs['shape'] = 'box'
attrs['shape'] = 'box'
Copy link
Member

Choose a reason for hiding this comment

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

is this making every shape a box?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably yes. Looks like I have an indentation error there. Quick question: when I fix this, should I use a different shape for Data than Deterministic? E.g., make Data be a roundtangle?

Copy link
Contributor Author

@rpgoldman rpgoldman May 27, 2019

Choose a reason for hiding this comment

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

OK, here's a fixed version and a sample output graph.
new-pymc3-graph

Fixing up the tests now.

Copy link
Contributor Author

@rpgoldman rpgoldman May 27, 2019

Choose a reason for hiding this comment

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

Tests should be fixed now. If they go, I propose to squash again, test again, and merge.

Copy link
Member

Choose a reason for hiding this comment

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

What about Potential nodes? I think they were triangles back in PyMC2.

Copy link
Contributor Author

@rpgoldman rpgoldman May 28, 2019

Choose a reason for hiding this comment

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

@fonnesbeck

What about Potential nodes? I think they were triangles back in PyMC2.

I don't mind having a whack at that, but I'd prefer to have it be a new issue and a new PR. If you would post a new issue about this, with a small model that has a potential in it, I'll have a look. But now I am going to squash this commit, and as long as that doesn't break anything, merge it and close this out.
[I don't mean to seem abrupt, but it is a lot of mental work -- at least for me -- to keep multiple topic branches open.]

@rpgoldman rpgoldman force-pushed the graph-data branch 2 times, most recently from f31d7f4 to 5455770 Compare May 28, 2019 01:17
@ColCarroll
Copy link
Member

Oy, sorry to keep tossing things your way @rpgoldman, but can you also add a line to RELEASE_NOTES.md? I think @twiecki is also trying to get 3.7 released, so you might even start a new section for pymc 3.8, and we can merge this right after the release.

@rpgoldman
Copy link
Contributor Author

Oy, sorry to keep tossing things your way @rpgoldman, but can you also add a line to RELEASE_NOTES.md? I think @twiecki is also trying to get 3.7 released, so you might even start a new section for pymc 3.8, and we can merge this right after the release.

@ColCarroll
OK, pushed new version with the release note.

@fonnesbeck Since the merge is going to be delayed, would you like to send me a tiny model with a Potential and I'll see if I can fit it into the grapher?

@rpgoldman
Copy link
Contributor Author

@fonnesbeck Triangles for potentials look pretty ugly IMO:
image
What about a different shape, such as hexagon, octagon or home, that would accommodate long text labels better?
image
I suspect of those, house and octagon will look most pleasing with a text label.

@rpgoldman
Copy link
Contributor Author

@fonnesbeck How about this instead?
image

Mark Data nodes in graph: previously these were incorrectly marked as
`Deterministic`.
Mark Potential nodes in graph.
Also distinguish between Deterministic, Data, and Potential nodes by node shape.

Put observations below observed nodes: Now that there are `Data`
objects that are graphable, they need to be placed correctly with
respect to other nodes: if they are inputs they should be above, and
if they are observations, they should be below.
@rpgoldman
Copy link
Contributor Author

If this passes tests once again, may I pretty-please merge it?

@fonnesbeck
Copy link
Member

Sorry for the slow reply. I think these look fantastic. Merging.

@fonnesbeck fonnesbeck merged commit 41e1e9d into pymc-devs:master May 31, 2019
@rpgoldman rpgoldman deleted the graph-data branch May 31, 2019 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants