Mark `pm.Data` nodes in graphviz graphs. #3491

rpgoldman · 2019-05-23T20:13:21Z

No description provided.

rpgoldman · 2019-05-23T22:34:39Z

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 · 2019-05-23T22:36:12Z

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

lucianopaz · 2019-05-24T07:54:25Z

pymc3/model_graph.py

@@ -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


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.

@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 · 2019-05-24T15:39:57Z

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

ColCarroll · 2019-05-24T16:06:34Z

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 · 2019-05-24T16:48:53Z

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 · 2019-05-24T16:49:05Z

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 · 2019-05-24T17:02:16Z

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 · 2019-05-27T15:36:30Z

Sorry to nag, but -- ok to merge?

ColCarroll

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

pymc3/model_graph.py

ColCarroll · 2019-05-27T15:53:29Z

pymc3/model_graph.py

        else:
            distribution = 'Deterministic'
-            attrs['shape'] = 'box'
+        attrs['shape'] = 'box'


is this making every shape a box?

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?

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

Fixing up the tests now.

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

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

@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.]

ColCarroll · 2019-05-28T03:45:52Z

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 · 2019-05-28T15:06:36Z

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 · 2019-05-28T15:52:57Z

@fonnesbeck Triangles for potentials look pretty ugly IMO:

What about a different shape, such as hexagon, octagon or home, that would accommodate long text labels better?

I suspect of those, house and octagon will look most pleasing with a text label.

rpgoldman · 2019-05-28T16:12:44Z

@fonnesbeck How about this instead?

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 · 2019-05-31T15:24:45Z

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

fonnesbeck · 2019-05-31T15:41:43Z

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

rpgoldman force-pushed the graph-data branch from e845c2f to 69db85f Compare May 23, 2019 20:14

lucianopaz reviewed May 24, 2019

View reviewed changes

rpgoldman force-pushed the graph-data branch from ee55466 to c5207dd Compare May 24, 2019 14:46

rpgoldman force-pushed the graph-data branch from 373a2bd to a51bc5e Compare May 24, 2019 17:00

rpgoldman added enhancements request discussion labels May 24, 2019

rpgoldman changed the title ~~[WIP] Mark pm.Data nodes in graphviz graphs.~~ Mark pm.Data nodes in graphviz graphs. May 27, 2019

ColCarroll reviewed May 27, 2019

View reviewed changes

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

rpgoldman force-pushed the graph-data branch from 5eb0c3d to 1e24fdf Compare May 30, 2019 22:18

rpgoldman force-pushed the graph-data branch from 1e24fdf to 9450ec7 Compare May 31, 2019 15:24

fonnesbeck merged commit 41e1e9d into pymc-devs:master May 31, 2019

rpgoldman deleted the graph-data branch May 31, 2019 17:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mark `pm.Data` nodes in graphviz graphs. #3491

Mark `pm.Data` nodes in graphviz graphs. #3491

rpgoldman commented May 23, 2019

rpgoldman commented May 23, 2019

rpgoldman commented May 23, 2019 •

edited

Loading

lucianopaz May 24, 2019

rpgoldman May 24, 2019

rpgoldman commented May 24, 2019

ColCarroll commented May 24, 2019

rpgoldman commented May 24, 2019

rpgoldman commented May 24, 2019

rpgoldman commented May 24, 2019

rpgoldman commented May 27, 2019

ColCarroll left a comment

ColCarroll May 27, 2019

rpgoldman May 27, 2019

rpgoldman May 27, 2019 •

edited

Loading

rpgoldman May 27, 2019 •

edited

Loading

fonnesbeck May 27, 2019

rpgoldman May 28, 2019 •

edited

Loading

ColCarroll commented May 28, 2019

rpgoldman commented May 28, 2019

rpgoldman commented May 28, 2019

rpgoldman commented May 28, 2019

rpgoldman commented May 31, 2019

fonnesbeck commented May 31, 2019

Mark pm.Data nodes in graphviz graphs. #3491

Mark pm.Data nodes in graphviz graphs. #3491

Conversation

rpgoldman commented May 23, 2019

rpgoldman commented May 23, 2019

rpgoldman commented May 23, 2019 • edited Loading

lucianopaz May 24, 2019

Choose a reason for hiding this comment

rpgoldman May 24, 2019

Choose a reason for hiding this comment

rpgoldman commented May 24, 2019

ColCarroll commented May 24, 2019

rpgoldman commented May 24, 2019

rpgoldman commented May 24, 2019

rpgoldman commented May 24, 2019

rpgoldman commented May 27, 2019

ColCarroll left a comment

Choose a reason for hiding this comment

ColCarroll May 27, 2019

Choose a reason for hiding this comment

rpgoldman May 27, 2019

Choose a reason for hiding this comment

rpgoldman May 27, 2019 • edited Loading

Choose a reason for hiding this comment

rpgoldman May 27, 2019 • edited Loading

Choose a reason for hiding this comment

fonnesbeck May 27, 2019

Choose a reason for hiding this comment

rpgoldman May 28, 2019 • edited Loading

Choose a reason for hiding this comment

ColCarroll commented May 28, 2019

rpgoldman commented May 28, 2019

rpgoldman commented May 28, 2019

rpgoldman commented May 28, 2019

rpgoldman commented May 31, 2019

fonnesbeck commented May 31, 2019

Mark `pm.Data` nodes in graphviz graphs. #3491

Mark `pm.Data` nodes in graphviz graphs. #3491

rpgoldman commented May 23, 2019 •

edited

Loading

rpgoldman May 27, 2019 •

edited

Loading

rpgoldman May 27, 2019 •

edited

Loading

rpgoldman May 28, 2019 •

edited

Loading