ellson / MOTHBALLED-graphviz

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

Can't seem to add multi edges in non-strict graph #1191

Closed chtenb closed 7 years ago

chtenb commented 7 years ago

Consider the following code:

void imdebug()
{
    Agraph_t* graph = agopen("test root graph", Agdirected, NULL);

    Agnode_t* node = agnode(graph, "node 1", 1);
    Agnode_t* node2 = agnode(graph, "node 2", 1);

    Agedge_t* edge1 = agedge(graph, node, node2, "edge 1", 1);
    Agedge_t* edge2 = agedge(graph, node, node2, "edge 2", 1);
    Agedge_t* edge3 = agedge(graph, node, node2, "edge 2", 1);
    Agedge_t* edge4 = agedge(graph, node, node2, "edge 3", 1);
    Agedge_t* edge5 = agedge(graph, node, node2, "edge 3", 1);
    Agedge_t* edge6 = agedge(graph, node, node2, "edge 1", 1);
    Agedge_t* edge7 = agedge(graph, node, node2, "edge 1", 1);

    cout
        << edge1 << ", "
        << edge2 << ", "
        << edge3 << ", "
        << edge4 << ", "
        << edge5 << ", "
        << edge6 << ", "
        << edge7 << endl;
}

Output:

08C63B40, 08C63B70, 08C63B70, 08C63B70, 08C63B70, 08C63B70, 08C63B70

As you can see, different edge names don't result in different edge pointers. Is something going wrong here, or do I overlook something?

magneticnorth commented 7 years ago

I’ll look at this. Your code looks right. (side question, what’s memDisc?)

On Jan 3, 2017, at 5:01 AM, Chiel ten Brinke notifications@github.com wrote:

Consider the following code:

void imdebug() { Agraph_t* graph = agopen("test root graph", Agdirected, &memDisc);

Agnode_t* node = agnode(graph, "node 1", 1);
Agnode_t* node2 = agnode(graph, "node 2", 1);

Agedge_t* edge1 = agedge(graph, node, node2, "edge 1", 1);
Agedge_t* edge2 = agedge(graph, node, node2, "edge 2", 1);
Agedge_t* edge3 = agedge(graph, node, node2, "edge 2", 1);
Agedge_t* edge4 = agedge(graph, node, node2, "edge 3", 1);
Agedge_t* edge5 = agedge(graph, node, node2, "edge 3", 1);
Agedge_t* edge6 = agedge(graph, node, node2, "edge 1", 1);
Agedge_t* edge7 = agedge(graph, node, node2, "edge 1", 1);

cout
    << edge1 << ", "
    << edge2 << ", "
    << edge3 << ", "
    << edge4 << ", "
    << edge5 << ", "
    << edge6 << ", "
    << edge7 << endl;

} Output:

08C63B40, 08C63B70, 08C63B70, 08C63B70, 08C63B70, 08C63B70, 08C63B70 As you can see, different edge names don't result in different edge pointers. Is something going wrong here, or do I overlook something?

— 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/1191, or mute the thread https://github.com/notifications/unsubscribe-auth/ACtWz17yCHLvYbSzZqF-UfwzE6MWl2_9ks5rOhyJgaJpZM4LZeZ3.

chtenb commented 7 years ago

It's not important. I replaced it by NULL in the sample code. The output remains the same.

emden commented 7 years ago

For the record, the graph

digraph { "node 1" -> "node 2" [key="edge 1"] "node 1" -> "node 2" [key="edge 2"] "node 1" -> "node 2" [key="edge 3"] }

has 3 distinct edges.

emden commented 7 years ago

Here's a trace of what happens in the calls to agedge:

agedge "node 1" "node 2" key "edge 1" have_id 1 create globally 0x103b950 agedge "node 1" "node 2" key "edge 2" have_id 1 found locally 0x103b990 agedge "node 1" "node 2" key "edge 2" have_id 1 found locally 0x103b990 agedge "node 1" "node 2" key "edge 3" have_id 1 found locally 0x103b990 agedge "node 1" "node 2" key "edge 3" have_id 1 found locally 0x103b990 agedge "node 1" "node 2" key "edge 1" have_id 1 found locally 0x103b990 agedge "node 1" "node 2" key "edge 1" have_id 1 found locally 0x103b990

I'm surprised that the keys are are already found via agmapnametoid. And why are "edge 2" and "edge 3" found locally? Where are they created?

Again for the record, the trace for the parsed graph is:

agedge "node 1" "node 2" key "edge 1" have_id 1 create globally 0x63ed90 agedge "node 1" "node 2" key "edge 2" have_id 1 create globally 0x63eec0 agedge "node 1" "node 2" key "edge 3" have_id 1 create globally 0x63eff0

magneticnorth commented 7 years ago

It appears someone broke cgraph a while back.

If you use graphviz/lib/cgraph from graphviz-LAST_LIBGRAPH.tar.gz then the test program works correctly.

I would like to dig deeper but can't see versions before June 2016 in www.github.com/ellson/graphviz though probably I'm doing something wrong. ubuntu@ip-54-40-18-147:~/src/graphviz$ git checkout 'master@{2015-12-01 00:00:00}' warning: Log for 'master' only goes back to Thu, 19 May 2016 16:28:45 +0000.

I see there was a lot of cleanup in cgraph/id.c and its friends but someone may have inadvertently broken it.

emden commented 7 years ago

There was just one change to id.c since 2012 and 3 relevant changes to edge.c.

emden commented 7 years ago

What is the version for graphviz-LAST_LIBGRAPH.tar.gz? I'm seeing problems even in 2.38:

$ LD_LIBRARY_PATH=$GROOT/lib ./a.out version 2.38.0 1 0x83f950, 2 0x83fa80, 2 0x83fac0, 3 0x83fbb0, 3 0x83fbf0, 1 0x83f990, 1 0x83f990

ellson commented 7 years ago

Using "touching paths" search in "gitk" on a local git clone of graphviz on Linux has no problems seeing older changes.

I only see 2 changes to lib/cgraph/id.c in the last 5 years:

895bdd619f51a0f46d79a290b0b487b4561ad183 add agregister() and idregister to id discipline - 2012-08-13

9cd94c3f5e9914a64e3f7a082c3eaba124348016 Fixed ID type for Win64 - 2015-02-04

John

On January 3, 2017 at 10:52 AM Stephen North notifications@github.com wrote:

It appears someone broke cgraph a while back.

If you use graphviz/lib/cgraph from graphviz-LAST_LIBGRAPH.tar.gz then the
test program works correctly.

I would like to dig deeper but can't see versions before June 2016 in
www.github.com/ellson/graphviz though probably I'm doing something wrong.
ubuntu@ip-54-40-18-147:~/src/graphviz$ git checkout 'master@{2015-12-01
00:00:00}'
warning: Log for 'master' only goes back to Thu, 19 May 2016 16:28:45 +0000.

I see there was a lot of cleanup in cgraph/id.c and its friends but someone
may have inadvertently broken it.

—
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/1191#issuecomment-270145950 , or mute the thread https://github.com/notifications/unsubscribe-auth/ABcTPaPelvIxdQNsrvjzs1fUSyQe_-Glks5rOm6_gaJpZM4LZeZ3 .
magneticnorth commented 7 years ago

Well, er, it's not that hard to see that the following function in edge.c is broken, since the comment (yes, a sighting of a rare meaningful comment in graphviz source, like catching a glimpse of a rare bird in the wild) says that agedgeifcmpf() accepts AGTYPE(e)==0 as a sort of wildcard, but the new and improved and cleaned up for WINDOWS VISTA implementation does not use AGTYPE(e), but the old version of this function did. This was due to a typo when cleaning up the code.

I uploaded the obvious fix just now. These things happen. Thank you for catching it and asking. Surprising the bug was lurking for so long.

/ edge comparison. OBJTYPE(e) == 0 means ID is a wildcard. / int agedgeidcmpf(Dict_t d, void arg_e0, void arg_e1, Dtdisc_t disc) { Agedge_t e0, e1;

NOTUSED(d);
e0 = arg_e0;
e1 = arg_e1;
NOTUSED(disc);

if (AGID(e0->node) < AGID(e1->node)) return -1;
if (AGID(e0->node) > AGID(e1->node)) return 1;
/* same node */
if ((AGID(e0) != 0) && (AGID(e1) != 0)) {
    if (AGID(e0) < AGID(e1)) return -1;
    if (AGID(e0) > AGID(e1)) return 1;
}
return 0;

}

On Tue, Jan 3, 2017 at 12:23 PM, John Ellson notifications@github.com wrote:

Using "touching paths" search in "gitk" on a local git clone of graphviz on Linux has no problems seeing older changes.

I only see 2 changes to lib/cgraph/id.c in the last 5 years:

895bdd619f51a0f46d79a290b0b487b4561ad183 add agregister() and idregister to id discipline - 2012-08-13

9cd94c3f5e9914a64e3f7a082c3eaba124348016 Fixed ID type for Win64 - 2015-02-04

John

On January 3, 2017 at 10:52 AM Stephen North notifications@github.com wrote:

It appears someone broke cgraph a while back.

If you use graphviz/lib/cgraph from graphviz-LAST_LIBGRAPH.tar.gz then the test program works correctly.

I would like to dig deeper but can't see versions before June 2016 in www.github.com/ellson/graphviz though probably I'm doing something wrong. ubuntu@ip-54-40-18-147:~/src/graphviz$ git checkout 'master@{2015-12-01 00:00:00}' warning: Log for 'master' only goes back to Thu, 19 May 2016 16:28:45 +0000.

I see there was a lot of cleanup in cgraph/id.c and its friends but someone may have inadvertently broken it.

— 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/1191#issuecomment-270145950 , or mute the thread https://github.com/notifications/unsubscribe-auth/ ABcTPaPelvIxdQNsrvjzs1fUSyQe_-Glks5rOm6_gaJpZM4LZeZ3 .

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/ellson/graphviz/issues/1191#issuecomment-270169854, or mute the thread https://github.com/notifications/unsubscribe-auth/ACtWzy5-H3K8TiEcOlLjP3FAA6-L-xASks5rOoQngaJpZM4LZeZ3 .

emden commented 7 years ago

Closer, but with the change, I still get:

1 0x9eb950, 2 0x9eba80, 2 0x9ebac0, 3 0x9ebbb0, 3 0x9ebbf0, 1 0x9eb990, 1 0x9eb990

include

include

/ extern char AgraphVersion[]; /

main() { Agraph_t* graph = agopen("test root graph", Agdirected, NULL);

Agnode_t node = agnode(graph, "node 1", 1); Agnode_t node2 = agnode(graph, "node 2", 1);

Agedge_t edge1 = agedge(graph, node, node2, "edge 1", 1); Agedge_t edge2 = agedge(graph, node, node2, "edge 2", 1); Agedge_t edge3 = agedge(graph, node, node2, "edge 2", 1); Agedge_t edge4 = agedge(graph, node, node2, "edge 3", 1); Agedge_t edge5 = agedge(graph, node, node2, "edge 3", 1); Agedge_t edge6 = agedge(graph, node, node2, "edge 1", 1); Agedge_t* edge7 = agedge(graph, node, node2, "edge 1", 1);

/ printf ("version %s\n", AgraphVersion); / printf("1 %p, 2 %p, 2 %p, 3 %p, 3 %p, 1 %p, 1 %p\n", edge1, edge2, edge3, edge4, edge5, edge6, edge7); }

The creation is working now, but the lookups still have problems.

If I put print statements in agedge(), I get

aginit "node 1" "node 2" key "edge 1" have_id 1 my_id 0 create global 0x9eb950 aginit "node 1" "node 2" key "edge 2" have_id 1 my_id 0 create global 0x9eba80 aginit "node 1" "node 2" key "edge 2" have_id 1 my_id 10402400 find local 0x9ebac0 aginit "node 1" "node 2" key "edge 3" have_id 1 my_id 0 create global 0x9ebbb0 aginit "node 1" "node 2" key "edge 3" have_id 1 my_id 10402704 find local 0x9ebbf0 aginit "node 1" "node 2" key "edge 1" have_id 1 my_id 10402096 find local 0x9eb990 aginit "node 1" "node 2" key "edge 1" have_id 1 my_id 10402096 find local 0x9eb990

magneticnorth commented 7 years ago

I believe all is well.

The test is an undirected graph, but each edge is actually an edge pair (source-target and target-source) so it’s possible to get either of them. If you print AGID(e) you can see the results are correct. Also agnedges(graph)==3 which is right. I recall at one point we (John? Emden?) did some work to canonicalize the results of the edge lookup functions but I’m not certain of the details.

On Jan 3, 2017, at 1:56 PM, emden notifications@github.com wrote:

Closer, but with the change, I still get:

1 0x9eb950, 2 0x9eba80, 2 0x9ebac0, 3 0x9ebbb0, 3 0x9ebbf0, 1 0x9eb990, 1 0x9eb990

include

include

/ extern char AgraphVersion[]; /

main() { Agraph_t* graph = agopen("test root graph", Agdirected, NULL);

Agnode_t node = agnode(graph, "node 1", 1); Agnode_t node2 = agnode(graph, "node 2", 1);

Agedge_t edge1 = agedge(graph, node, node2, "edge 1", 1); Agedge_t edge2 = agedge(graph, node, node2, "edge 2", 1); Agedge_t edge3 = agedge(graph, node, node2, "edge 2", 1); Agedge_t edge4 = agedge(graph, node, node2, "edge 3", 1); Agedge_t edge5 = agedge(graph, node, node2, "edge 3", 1); Agedge_t edge6 = agedge(graph, node, node2, "edge 1", 1); Agedge_t* edge7 = agedge(graph, node, node2, "edge 1", 1);

/ printf ("version %s\n", AgraphVersion); / printf("1 %p, 2 %p, 2 %p, 3 %p, 3 %p, 1 %p, 1 %p\n", edge1, edge2, edge3, edge4, edge5, edge6, edge7); }

The creation is working now, but the lookups still have problems.

If I put print statements in agedge(), I get

aginit "node 1" "node 2" key "edge 1" have_id 1 my_id 0 create global 0x9eb950 aginit "node 1" "node 2" key "edge 2" have_id 1 my_id 0 create global 0x9eba80 aginit "node 1" "node 2" key "edge 2" have_id 1 my_id 10402400 find local 0x9ebac0 aginit "node 1" "node 2" key "edge 3" have_id 1 my_id 0 create global 0x9ebbb0 aginit "node 1" "node 2" key "edge 3" have_id 1 my_id 10402704 find local 0x9ebbf0 aginit "node 1" "node 2" key "edge 1" have_id 1 my_id 10402096 find local 0x9eb990 aginit "node 1" "node 2" key "edge 1" have_id 1 my_id 10402096 find local 0x9eb990

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/ellson/graphviz/issues/1191#issuecomment-270193129, or mute the thread https://github.com/notifications/unsubscribe-auth/ACtWz1p0PZw7GAoFb0RQlA0n1PwVNQPEks5rOpn7gaJpZM4LZeZ3.

chtenb commented 7 years ago

It seems everything is working now indeed. Thanks for the swift response!

emden commented 7 years ago

Ah, I forgot about that. All told, your conversion from agraph to cgraph made things much more consistent. The only gotcha is having two pointers for an edge, which I often forget about.

magneticnorth commented 7 years ago

We could work harder to choose one canonical representation for random access.

On Wed, Jan 4, 2017 at 11:01 AM, emden notifications@github.com wrote:

Ah, I forgot about that. All told, your conversion from agraph to cgraph made things much more consistent. The only gotcha is having two pointers for an edge, which I often forget about.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/ellson/graphviz/issues/1191#issuecomment-270407565, or mute the thread https://github.com/notifications/unsubscribe-auth/ACtWz39NO6i3q4xl6cUTgDlcJ7rE76RPks5rO8JRgaJpZM4LZeZ3 .