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 )
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)
Change History (18)
comment:1 Changed 10 years ago by
Changed 8 years ago by
comment:2 Changed 8 years ago by
- 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
- 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
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 8 years ago by
What do you say to this adjustment? :-)
Lukáš.
comment:6 in reply to: ↑ 5 Changed 8 years ago by
- 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
comment:8 Changed 8 years ago by
comment:9 Changed 8 years ago by
Please state clearly which patches have to be applied.
comment:11 Changed 8 years ago by
(Just adding a proper commit message.)
comment:12 Changed 8 years ago by
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
- Status changed from positive_review to needs_work
Changed 8 years ago by
Changed 8 years ago by
comment:14 Changed 8 years ago by
- Status changed from needs_work to positive_review
Fixed too :-)
Nathann
comment:15 Changed 8 years ago by
- Merged in set to sage-5.0.beta5
- Resolution set to fixed
- Status changed from positive_review to closed
Easy to fix, just replace (on line 944 of
graph.py
)with something like