Opened 10 years ago

Closed 8 years ago

#9714 closed defect (fixed)

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

Reported by: was Owned by: jason, ncohen, rlm
Priority: minor Milestone: sage-5.0
Component: graph theory Keywords:
Cc: Merged in: sage-5.0.beta5
Authors: Lukáš Lánský, Robert Miller Reviewers: Nathann Cohen
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by brunellus)

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:

Attachments (3)

trac_9714_incidence_checking.patch (3.0 KB) - added by brunellus 8 years ago.
trac_9714_review.patch (1.1 KB) - added by ncohen 8 years ago.
trac_9714_review_review.patch (795 bytes) - added by ncohen 8 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 10 years ago by rlm

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

Changed 8 years ago by brunellus

comment:2 Changed 8 years ago by brunellus

  • Status changed from new to needs_review

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

comment:3 Changed 8 years ago by ncohen

  • Reviewers set to 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

comment:4 Changed 8 years ago by brunellus

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.

comment:5 follow-up: Changed 8 years ago by brunellus

What do you say to this adjustment? :-)

Lukáš.

comment:6 in reply to: ↑ 5 Changed 8 years ago by ncohen

  • Status changed from needs_review to positive_review

What do you say to this adjustment? :-)

"Stupid me"

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

Nathann

comment:7 Changed 8 years ago by brunellus

  • Authors set to Lukáš Lánský

comment:8 Changed 8 years ago by brunellus

  • Authors changed from Lukáš Lánský to Lukáš Lánský, Robert Miller

comment:9 Changed 8 years ago by jdemeyer

Please state clearly which patches have to be applied.

comment:10 Changed 8 years ago by brunellus

  • Description modified (diff)

Oh, sorry. :-)

comment:11 Changed 8 years ago by brunellus

(Just adding a proper commit message.)

comment:12 Changed 8 years ago by jdemeyer

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.

comment:13 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work

Changed 8 years ago by ncohen

Changed 8 years ago by ncohen

comment:14 Changed 8 years ago by ncohen

  • Status changed from needs_work to positive_review

Fixed too :-)

Nathann

comment:15 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.0.beta5
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.