Opened 12 years ago

Closed 11 years ago

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

Reported by: Owned by: was jason, ncohen, rlm minor sage-5.0 graph theory sage-5.0.beta5 Lukáš Lánský, Robert Miller Nathann Cohen N/A

We have

```sage: M = matrix(3, [1,2,0, 0,2,0, 0,0,1])
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:

### comment:1 Changed 12 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
```

### comment:2 Changed 11 years ago by brunellus

Status: new → 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 11 years ago by ncohen

Reviewers: → 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 11 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:  6 Changed 11 years ago by brunellus

What do you say to this adjustment? :-)

Lukáš.

### comment:6 in reply to:  5 Changed 11 years ago by ncohen

Status: needs_review → positive_review

What do you say to this adjustment? :-)

"Stupid me"

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

Nathann

### comment:7 Changed 11 years ago by brunellus

Authors: → Lukáš Lánský

### comment:8 Changed 11 years ago by brunellus

Authors: Lukáš Lánský → Lukáš Lánský, Robert Miller

### comment:9 Changed 11 years ago by jdemeyer

Please state clearly which patches have to be applied.

### comment:10 Changed 11 years ago by brunellus

Description: modified (diff)

Oh, sorry. :-)

### comment:11 Changed 11 years ago by brunellus

(Just adding a proper commit message.)

### comment:12 Changed 11 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 11 years ago by jdemeyer

Status: positive_review → needs_work

### Changed 11 years ago by ncohen

Fixed too `:-)`