Correct and complete logic to prevent node duplication.

[joseph-r-hamilton]

Greetings. I am building a graphml adapter for pycallgraph. I chose to use pyyed. One of my bugs generated duplicate nodes and I was surprised pyyed didn't catch it. It does now.

[bockor]

Dear Joe --

Regarding the rules you're not familiar with. yEd is merely applying the GraphML recommendations. In order to understand the concept of nested graphs I recommend to read and absorb [http://graphml.graphdrawing.org/primer/graphml-.html#Nestedprimer] . I am convinced you will get the idea.

Rest assured that the output file of the demo script I have posted on our conversation string is in line with this recommendation. Eventually use your browser (or any XML-Viewer) to analyse and verify the format.

I hope this contributes.

Regards,

Bruno

[bockor]

Joe --

Please use Nested Graphs Excuse me for eventual inconvenience.

Bruno

[joseph-r-hamilton]

I added a commit.

Take a look...

regards,

Joseph Hamilton

On Mon, Oct 26, 2020 at 1:45 PM bockor notifications@github.com wrote:

Joe --

Please use Nested Graphs http://graphml.graphdrawing.org/primer/graphml-primer.html#Nested Excuse me for eventual inconvenience.

Bruno

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jamesscottbrown/pyyed/pull/33#issuecomment-716750691, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIF2GIA3L5PHJ5QBIUQ2MATSMW7WJANCNFSM4S47N7OQ .

[bockor]

Joe --

This seems also to work. I have conducted a couple of test cases (e.g. https://github.com/bockor/pyyed-nested-graphs/tree/main/joe). Question: What's purpose of the variable "self.parent" ? in the Group & Node class. It is initiated, however never used IMHO.

On another note. In order to be consistent and avoid confusion, please change the "edge_color", "edge_type" and "edge_width" Node & Group Class attribute names to respectively "border_color", "border_type" and "border_width".

Regards,

Bruno

[joseph-r-hamilton]

Can you help me out a bit?

You mentioned nested graph edges opens up possibilities.

Can you explain that a bit more?

Right now, I'm pondering how to build hierarchical structures with pyyed. As per that documentation, the "safest" thing to do is to handle all the edges at the top-most graph level. I related the example pyyed script could be done that way.

What benefits would get to treat edges in the earliest ancestor rather than the top-level graph?

If it helps, for context, the entire reason I'm hacking away at this is that any moderately complex python program will create a call graph that is all but unusable when converted to a static image. Some people have created "zoom-in" to dot things. But it's still static. yEd permits me tons more functionality. Specific to hierarchies and call graphs, I can close/open groups as desired.

So there's a benefit to groups (and groups of groups). But I'm not seeing the same benefit for edges.

Thoughts?

regards,

Joseph Hamilton

On Tue, Oct 27, 2020 at 12:11 PM Joseph Hamilton < joseph.r.hamilton@gmail.com> wrote:

I added a commit.

Take a look...

regards,

Joseph Hamilton

On Mon, Oct 26, 2020 at 1:45 PM bockor notifications@github.com wrote:

Joe --

Please use Nested Graphs http://graphml.graphdrawing.org/primer/graphml-primer.html#Nested Excuse me for eventual inconvenience.

Bruno

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jamesscottbrown/pyyed/pull/33#issuecomment-716750691, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIF2GIA3L5PHJ5QBIUQ2MATSMW7WJANCNFSM4S47N7OQ .

[joseph-r-hamilton]

Bruno,

The parent attribute is set by the parent. It didn't seem prudent to include it as a parameter via initialization since it is possible (for whatever reason) to create Node or Group directly. It is not set by the top-level graph because when parent is None, this is the same as being a child of the top-level graph. So it's only set in Group.add_node and Group.add_group.

Then the parent attribute is used in the is_ancestor method to provide the validation for the nested edge so what pyyed produces will indeed load in yEd.

regards,

Joseph

On Wed, Oct 28, 2020 at 10:53 AM bockor notifications@github.com wrote:

Joe --

This seems also to work. I have conducted a couple of test cases (e.g. https://github.com/bockor/pyyed-nested-graphs/tree/main/joe). Question: What's purpose of the variable "self.parent" ? in the Group & Node class. It is initiated, however never used IMHO.

On another note. In order to be consistent and avoid confusion, please change the "edge_color", "edge_type" and "edge_width" Node & Group Class attribute names to respectively "border_color", "border_type" and "border_width".

Regards,

Bruno

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jamesscottbrown/pyyed/pull/33#issuecomment-718028704, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIF2GIGCXRMSFPJLKZTLOOTSNA5AVANCNFSM4S47N7OQ .

[bockor]

Joe --

You mentioned two hours ago:

''' ... Right now, I'm pondering how to build hierarchical structures with pyyed. As per that documentation, the "safest" thing to do is to handle all the edges at the top-most graph level. I related the example pyyed script could be done that way. What benefits would get to treat edges in the earliest ancestor rather than the top-level graph? ... So there's a benefit to groups (and groups of groups). But I'm not seeing the same benefit for edges. '''

I invite to try graphing https://github.com/bockor/pyyed-nested-graphs/blob/main/joe/infra_joe.png using the genuine pyyed library, which lacks the Group.add_edge() method.

In the same folder I have posted the respective script for the test: https://github.com/bockor/pyyed-nested-graphs/blob/main/joe/infra_pyyed.py ... and the rsulting graphml file https://github.com/bockor/pyyed-nested-graphs/blob/main/joe/infra_pyyed.graphml

When you try this to open this graphml in YEd it throws "java.lang.NullPointerException". Why ? Because the format / structure is not in line with http://graphml.graphdrawing.org/primer/graphml-.html#Nestedprimer

That's what I mean that this new feature offers opportunities to capture complex multi layer graphs. IMHO the traditional pyyed library was not build for this purpose. Or maybe I am not using the genuine pyyed library as it should ?!

Regards,

Bruno

[joseph-r-hamilton]

Bruno,

Problem here is bugs, not missing functionality.

The issue with the null pointer is the reason for my original PR. I was doing something similar - creating a bug in stuff calling pyyed which pyyed should have caught.

You have created groups with the same ID. I was creating nodes with the same ID. You're actually lucky. For me, yEd wasn't preventing the load. It WAS loading it... which left me rather confused for a while... since it was not doing what I expected.

If you fix the bug in infra_pyyed.py (line 22) then v 1.3 of pyyed will create output yEd will load. But... this is more like what I was experiencing earlier. The only difference is that my code was calling pyyed creating nodes with the same ID while as now, with v1.3, it's a bug in pyyed that does the same thing. Look at the XML that's generated. You'll see there are two nodes with id="SERVICES2". Why? Because in line 44 the g.add_edge call creates a top-level node for SERVICES2. Why? Due to the fact that the stuff checking for previously existing nodes wasn't working. Actually, this is exactly what my bug was doing. I was creating a group and a top-level node with the same ID. The check was failing and let me do that. Whereas for you, the check is failing and creating an extra node.

If you run infra_pyyed.py as is with my original PR, it will fail pointing out the problem with creating two groups with the same ID. If you change "SERVICES" to "SERVICES2" to fix your bug, it will create output that will match the PNG you've referenced.

So... that's the reason for my query. It's cool to have the new nested edge functionality. But, as far as I can tell, it will suffice always to define edges at the top-level. For the purposes of viewing graphs in yEd, I cannot see any difference in behavior. Otherwise stated, the Group.add_edge should never be required, especially since the documentation blatantly states the "safest" thing to do is to define all edges at the top level.

Now... there are things that could be considered if you wanted to make pyyed a bit more complex. Wouldn't it be nice to create two graphs separately and then "add" one inside the other? Or maybe delete a sub-graph? Maybe if you had a running process that dumped out graphs periodically? Maybe you would want to pull out a sub-graph and have it be its own graph. I dunno. Just brainstorming. All that's well beyond what I need at the moment.

regards,

Joseph

On Wed, Oct 28, 2020 at 12:52 PM bockor notifications@github.com wrote:

Joe --

You mentioned two hours ago:

'''

... Right now, I'm pondering how to build hierarchical structures with pyyed. As per that documentation, the "safest" thing to do is to handle all the edges at the top-most graph level. I related the example pyyed script could be done that way. What benefits would get to treat edges in the earliest ancestor rather than the top-level graph? ... So there's a benefit to groups (and groups of groups). But I'm not seeing the same benefit for edges. '''

I invite to try graphing https://github.com/bockor/pyyed-nested-graphs/blob/main/joe/infra_joe.png using the genuine pyyed library, which lacks the Group.add_edge() method.

In the same folder I have posted the respective script for the test: https://github.com/bockor/pyyed-nested-graphs/blob/main/joe/infra_pyyed.py ... and the rsulting graphml file

https://github.com/bockor/pyyed-nested-graphs/blob/main/joe/infra_pyyed.graphml

When you try this to open this graphml in YEd it throws "j ava.lang.NullPointerException". Why ? Because the format / structure is not in line with http://graphml.graphdrawing.org/primer/graphml-.html#Nestedprimer

That's what I mean that this new feature offers opportunities to capture complex multi layer graphs. IMHO the traditional pyyed library was not build for this purpose. Or maybe I am not using the genuine pyyed library as it should ?!

Regards,

Bruno

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jamesscottbrown/pyyed/pull/33#issuecomment-718105074, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIF2GIFAFAGZXLKYC66OHHTSNBK4VANCNFSM4S47N7OQ .

[bockor]

Joe --

Right, I have amended line 24 (not 22) in infra_pyyed.py.

Now I run infra_pyyed.py using genuine pyyed and load the grapml into yEd. --> https://github.com/bockor/pyyed-nested-graphs/blob/main/joe/infra_pyyed.png

Further I run infra_joe.py using pyyed_joe.py and load also the graphml into yEd --> https://github.com/bockor/pyyed-nested-graphs/blob/main/joe/infra_joe.png

Which one looks right ? I would say the latter one. On top of this when closing infra_pyyed.graphml it throws again the famous "java.lang.NullPointerException".

We are in the right direction I must say.

Regards,

Bruno

[bockor]

Dear Joe -- Congrats for instituting these signifcant improvements! Hopefully we will see them very soon committed and made available on the PyPI repository. Regards, Bruno

[bockor]

Dear Joe -- Congrats for instituting these signifcant improvements! Hopefully we will see them very soon committed and made available on the PyPI repository. Regards, Bruno

[bockor]

Joe -- Please change Line 227: # edge options --> # border options Regards, Bruno

[bockor]

Joe -- Regarding line 122: raise RuntimeWarning(f"""Group { Where you are using f-strings (formatted string literals). f-strings have been introduced in python 3.6. Despite it's a cool feature I am afraid it will throw a SyntaxError for python < 3.6 users. Regards, Bruno

[jamesscottbrown]

These changes have been merged via https://github.com/jamesscottbrown/pyyed/pull/36 (pulling to a new branch on my repo then making a new PR allowed me to replace the f-string).

[jamesscottbrown]

Thanks for these improvements, @bockor and @joseph-r-hamilton.