ellson / MOTHBALLED-graphviz

Moved to https://gitlab.com/graphviz/graphviz
Eclipse Public License 1.0
1.29k stars 255 forks source link

agedge() not using name? #1252

Open cdeccio opened 7 years ago

cdeccio commented 7 years ago

pygraphviz's get_edge() calls libgraphviz's agedge() under the hood. Some behavior seemed to change with get_edge(), between version 2.38 and 2.39. Specifically, if I pass a key to get_edge() (which also gets passed as the "name" argument to agedge()) with version 2.39, it returns an edge, even if the key/name doesn't match. In 2.38 the same call would return KeyError. Of course, the latter is the expected/desired behavior.

Here is the pygraphviz code to reproduce it:

Version 2.38:

from pygraphviz import AGraph g = AGraph(directed=True) g.add_edge('a','b','123') g.get_edge('a','b','456') Traceback (most recent call last): ... KeyError: 'Edge a-b not in graph.'

Version 2.39:

from pygraphviz import AGraph g = AGraph(directed=True) g.add_edge('a','b','123') g.get_edge('a','b','456') (u'a', u'b')

Was this an intended API change or a regression?

magneticnorth commented 7 years ago

I’m not familiar with pygraphviz. I tried to install it locally via pip and pip3, but the build fails. It’s probably my fault, but building 'pygraphviz._graphviz' extension creating build/temp.macosx-10.12-x86_64-3.6 creating build/temp.macosx-10.12-x86_64-3.6/pygraphviz clang -Wno-unused-result -Wsign-compare -Wunreachable-code -fno-common -dynamic -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -I/Users/north/include/graphviz -I/usr/local/include -I/usr/local/opt/openssl/include -I/usr/local/opt/sqlite/include -I/usr/local/Cellar/python3/3.6.1/Frameworks/Python.framework/Versions/3.6/include/python3.6m -c pygraphviz/graphviz_wrap.c -o build/temp.macosx-10.12-x86_64-3.6/pygraphviz/graphviz_wrap.o pygraphviz/graphviz_wrap.c:2954:10: fatal error: 'graphviz/cgraph.h' file not found

include "graphviz/cgraph.h"

         ^
1 error generated.
error: command 'clang' failed with exit status 1

Moving on, I’m not sure if pygraphviz uses graphviz/lib/agraph which I believe has been deprecated to the extent that it is no longer built or used by any of the graphviz programs. These days the tools all use cgraph. You can try the following program in cgraph and confirm it does what is expected:

include

include "cgraph.h"

int main(int argc, char *argv) { Agraph_t g; Agnode_t a, b; Agedge_t e, f;

g = agopen("test",Agdirected,0);
a = agnode(g,"a",TRUE);
b = agnode(g,"b",TRUE);
e = agedge(g,a,b,"123",TRUE);
f = agedge(g,a,b,"456",FALSE);

    fprintf(stderr,"%lx %lx\n",e,f);

}

Also, from “git log lib/agraph” I don’t believe anyone has been working there in a long time.

So, I’m not sure how to move forward here.

I did some patches in lib/cgraph in January, e.g. commit dbe61e9fea9b254c046577f36d5e9a3150e2c002 Author: Stephen C North scnorth@gmail.com Date: Tue Jan 3 12:55:00 2017 -0500

Fixed typo that broke wildcard feature in agedgeidcmpf()

This would seem possibly related, except it’s cgraph not agraph.

Stephen North

On Jun 28, 2017, at 4:31 PM, Casey Deccio notifications@github.com wrote:

pygraphviz's get_edge() calls libgraphviz's agedge() under the hood. Some behavior seemed to change with get_edge(), between version 2.38 and 2.39. Specifically, if I pass a key to get_edge() (which also gets passed as the "name" argument to agedge()) with version 2.39, it returns an edge, even if the key/name doesn't match. In 2.38 the same call would return KeyError. Of course, the latter is the expected/desired behavior.

Here is the pygraphviz code to reproduce it:

Version 2.38:

from pygraphviz import AGraph g = AGraph(directed=True) g.add_edge('a','b','123') g.get_edge('a','b','456') Traceback (most recent call last): ... KeyError: 'Edge a-b not in graph.'

Version 2.39:

from pygraphviz import AGraph g = AGraph(directed=True) g.add_edge('a','b','123') g.get_edge('a','b','456') (u'a', u'b')

Was this an intended API change or a regression?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/ellson/graphviz/issues/1252, or mute the thread https://github.com/notifications/unsubscribe-auth/ACtWz5JX-JWiuwCKNUfiy1yE9veXfAG4ks5sIrgggaJpZM4OIfR-.

cdeccio commented 7 years ago

Thanks for making the effort to try to get a pygraphviz environment up and running. I'm not sure how to help with that. I just installed graphviz (2.40) with MacPorts and then ran "python setup.py build" to build pygraphviz.

As far as cgraph and agraph are concerned, looking at setup.py indicates that cgraph is being used:

libraries=["cdt", "cgraph"],

I can also see from the errors from your attempted build above that cgraph is being referenced:

include "graphviz/cgraph.h"

But back to the issue at hand, your C example code worked just fine, but the pygraphviz code just doesn't seem to get the same result. I'm still unsure if it's pygraphviz or graphviz at this point. I'm going to poke around a bit more. Maybe @hagberg sees something I'm missing.

hagberg commented 7 years ago

@magneticnorth discovered the issue in graphviz 2.40 which has been already fixed in the current development versions of graphviz. e.g. with graphviz version 2.41.20170626.1516 (20170626.1516)

--------------
agedge OK
----------------
$ cat test.c; /usr/local/bin/dot -V; gcc test.c -L/usr/local/lib/graphivz -I/usr/local/include/graphviz -lcgraph -lcdt; ./a.out
#include <stdio.h>
#include "cgraph.h"

int main(int argc, char **argv)
{
    Agraph_t *g;
    Agnode_t *a, *b;
    Agedge_t *e, *f;

    g = agopen("test",Agdirected,0);
    a = agnode(g,"a",TRUE);
    b = agnode(g,"b",TRUE);
    e = agedge(g,a,b,"123",TRUE);
    f = agedge(g,a,b,"456",FALSE);

                fprintf(stderr,"%lx %lx\n",e,f);
}
dot - graphviz version 2.41.20170626.1516 (20170626.1516)
test.c: In function ‘main’:
test.c:16:35: warning: format ‘%lx’ expects argument of type ‘long unsigned int’, but argument 3 has type ‘Agedge_t * {aka struct Agedge_s *}’ [-Wformat=]
                 fprintf(stderr,"%lx %lx\n",e,f);
                                   ^
test.c:16:39: warning: format ‘%lx’ expects argument of type ‘long unsigned int’, but argument 4 has type ‘Agedge_t * {aka struct Agedge_s *}’ [-Wformat=]
                 fprintf(stderr,"%lx %lx\n",e,f);
                                       ^
55677d962950 0

-------------------------
 pygraphviz works as expected

$ cat tg.py;python tg.py
import os,sys
from pygraphviz import AGraph
print(sys.version)
os.system('dot -V')
g = AGraph(directed=True)
g.add_edge('a','b','123')
g.get_edge('a','b','456')
3.6.1 |Anaconda 4.4.0 (64-bit)| (default, May 11 2017, 13:09:58)
[GCC 4.4.7 20120313 (Red Hat 4.4.7-1)]
dot - graphviz version 2.41.20170626.1516 (20170626.1516)
Traceback (most recent call last):
  File "/home/aric/.local/lib/python3.6/site-packages/pygraphviz/agraph.py", line 1640, in __new__
    _Action.find)
KeyError: 'agedge: no key'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "tg.py", line 7, in <module>
    g.get_edge('a','b','456')
  File "/home/aric/.local/lib/python3.6/site-packages/pygraphviz/agraph.py", line 509, in get_edge
    return Edge(self, u, v, key)
  File "/home/aric/.local/lib/python3.6/site-packages/pygraphviz/agraph.py", line 1642, in __new__
    raise KeyError("Edge %s-%s not in graph." % (source, target))
KeyError: 'Edge a-b not in graph.'
cdeccio commented 7 years ago

Thanks, @hagberg , for shedding some light on this. I confirmed that it is fixed in the development version.

I saw the problem with 2.39 and also with 2.40, though the instance of 2.38 that I was using didn't seem to exhibit the issue [1]. I'm not sure if the fixes might be backported for systems that might be using the 2.39.x and 2.40.x releases.

[1] https://groups.google.com/forum/#!topic/pygraphviz-discuss/rkoqKhN-R9o