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

[William Stein]

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:

• trac_9714_incidence_checking.patch​
• trac_9714_review.patch​
• trac_9714_review_review.patch​

[Robert Miller]

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

[Lukáš Lánský]

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

[Nathann Cohen]

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

[Lukáš Lánský]

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.

[Lukáš Lánský]

What do you say to this adjustment? :-)

Lukáš.

[Nathann Cohen]

What do you say to this adjustment? :-)

"Stupid me"

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

Nathann

[Jeroen Demeyer]

Please state clearly which patches have to be applied.

[Lukáš Lánský]

Oh, sorry. :-)

[Lukáš Lánský]

(Just adding a proper commit message.)

[Jeroen Demeyer]

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.

[Nathann Cohen]

Fixed too :-)

Nathann