Graph(..., format='incidence_matrix') doesn't work with graphs that have loops, but G.incidence_matrix() does. So?

[williamstein]

We have

sage: M = matrix(3, [1,2,0, 0,2,0, 0,0,1])
sage: g = Graph(M, format='adjacency_matrix')
sage: I = g.incidence_matrix(); I
[-1 -1  0  0  0  1]
[ 1  1  0  1  1  0]
[ 0  0  1  0  0  0]

But then:

sage: Graph(I, format='incidence_matrix').show(graph_border=True)
kaboom!

Either the first .incidence_matrix() should fail, or the second Graph(...) should work.

Apply:

• attachment: trac_9714_incidence_checking.patch
• attachment: trac_9714_review.patch
• attachment: trac_9714_review_review.patch

Component: graph theory

Author: Lukáš Lánský, Robert Miller

Reviewer: Nathann Cohen

Merged: sage-5.0.beta5

Issue created by migration from https://trac.sagemath.org/ticket/9714

[rlmill]

comment:1

Easy to fix, just replace (on line 944 of graph.py)

if len(NZ) != 2:
    msg += "There must be two nonzero entries (-1 & 1) per column."
    assert False

with something like

if len(NZ) == 1:
    if loops is None:
        loops = True
    elif not loops:
        msg += "There must be two nonzero entries (-1 & 1) per column."
        assert False
elif len(NZ) != 2:
    msg += "There must be two nonzero entries (-1 & 1) per column."
    assert False

[sagetrac-brunellus]

Attachment: trac_9714_incidence_checking.patch.gz

[sagetrac-brunellus]

comment:2

Then there is another problem: checking forgets possibility that there are only two vertices defined. I tried to fix that: see the second doctest.

[nathanncohen]

Reviewer: Nathann Cohen

[nathanncohen]

comment:3

Helloooooooooooooooo !!!

I find a bit weird that this code deals with -1 and 1 entries for undirected graphs, but well... ^^;

Anyway, here is a very small patch that just avoid some unnecessary computations.

I give a positive review to your patch, and you can review mine if you have some time :-)

Nathann

[sagetrac-brunellus]

comment:4

Hi, thanks for the review. You are certainly right that -1 is weird thing in this context and constructor should accept a normal incidence matrix with two ones in each column. I'll start another ticket for this.

I'll set positive review as soon as the tests pass.

[sagetrac-brunellus]

comment:5

What do you say to this adjustment? :-)

Lukáš.

[nathanncohen]

comment:6

What do you say to this adjustment? :-)

"Stupid me"

Ok, now it's good to go :-)

Nathann

[sagetrac-brunellus]

Author: Lukáš Lánský

[sagetrac-brunellus]

Changed author from Lukáš Lánský to Lukáš Lánský, Robert Miller

[jdemeyer]

comment:9

Please state clearly which patches have to be applied.

[sagetrac-brunellus]

comment:10

Oh, sorry. :-)

[sagetrac-brunellus]

comment:11

(Just adding a proper commit message.)

[jdemeyer]

comment:12

The last two patches have one annoyingly long line as commit message. Could you please shorten the line length. Multiple lines are allowed, but the first line should make sense by itself.

[nathanncohen]

Attachment: trac_9714_review.patch.gz

[nathanncohen]

comment:14

Attachment: trac_9714_review_review.patch.gz

Fixed too :-)

Nathann

[jdemeyer]

Merged: sage-5.0.beta5