Opened 9 months ago
Closed 4 months ago
#15545 closed defect (fixed)
TypeError in matching_polynomial
Reported by: | rlm | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.3 |
Component: | graph theory | Keywords: | |
Cc: | ncohen | Merged in: | |
Authors: | Ralf Stephan | Reviewers: | Nathann Cohen |
Report Upstream: | N/A | Work issues: | |
Branch: | 0f36ea8 (Commits) | Commit: | 0f36ea8c8330288a2da5c0eb135881fc7ed5612c |
Dependencies: | Stopgaps: |
Description
sage: G = Graph(10); G Graph on 10 vertices sage: G.matching_polynomial() x^10 sage: G.add_vertex((0,1)) sage: G.matching_polynomial() --------------------------------------------------------------------------- TypeError Traceback (most recent call last) <ipython-input-68-174221329fce> in <module>() ----> 1 G.matching_polynomial() /Users/miller/sage-5.10/local/lib/python2.7/site-packages/sage/graphs/matchpoly.so in sage.graphs.matchpoly.matching_polynomial (sage/graphs/matchpoly.c:2681)() TypeError: an integer is required
Another victim of arbitrary vertex labels...
Change History (16)
comment:1 Changed 8 months ago by vbraun_spam
- Milestone changed from sage-6.1 to sage-6.2
comment:2 Changed 5 months ago by vbraun_spam
- Milestone changed from sage-6.2 to sage-6.3
comment:3 Changed 4 months ago by rws
- Priority changed from minor to major
comment:4 follow-up: ↓ 8 Changed 4 months ago by ncohen
The fix is easy : check if the graph is labelled right, and if not call the function on a relabelled version.
Aaaaaaand I wonder what you think other Sage bugs are if you believe that this is a "major" problem :-P
Nathann
comment:5 follow-up: ↓ 6 Changed 4 months ago by rws
I don't think complexity is what is meant there. I mean any software requirement can lead to a complex change. Rather it's the perceived importance, and the bug reporter, esp. if new to sage, should be taken seriously. You want sage used by many people. So please be verbose with error messages too.
comment:6 in reply to: ↑ 5 ; follow-up: ↓ 7 Changed 4 months ago by ncohen
I don't think complexity is what is meant there. I mean any software requirement can lead to a complex change. Rather it's the perceived importance, and the bug reporter, esp. if new to sage, should be taken seriously. You want sage used by many people.
I don't mean that it shouldn't be fixed, of course. I am just saying that I can easily name a couple of things much more important to be fixed than this :-P
So please be verbose with error messages too.
You know the bug and you know the fix, please don't make me fix it for you. I will be the reviewer, but you know how to write this.
Nathann
comment:7 in reply to: ↑ 6 Changed 4 months ago by rws
So please be verbose with error messages too.
You know the bug and you know the fix, please don't make me fix it for you. I will be the reviewer, but you know how to write this.
Of course but I didn't want to imply you should do the fix. I assumed you were the original author and wanted to make you do better next time.
comment:8 in reply to: ↑ 4 Changed 4 months ago by rws
Replying to ncohen:
The fix is easy : check if the graph is labelled right, and if not call the function on a relabelled version.
Bummer, that code works fine when run from the Sage command line:
L = [] for v, d in G.degree_iterator(labels=True): L.append((d, v)) <--------- error message L.sort() d = {} for i from 0 <= i < nverts: d[L[i][1]] = i G = G.relabel(d, inplace=False)
So, is this a Cython problem?
comment:9 Changed 4 months ago by rws
- Branch set to u/rws/typeerror_in_matching_polynomial
comment:10 Changed 4 months ago by rws
- Commit set to e295da30f2ccbd64b5fdbed4571333ad7aeb816b
- Status changed from new to needs_review
Most minimal fix I did so far. Simply a bug in Cython usage. Please review.
New commits:
e295da3 | 15545: do not use Cython var for unrelated purposes |
comment:11 Changed 4 months ago by git
- Commit changed from e295da30f2ccbd64b5fdbed4571333ad7aeb816b to a8766b07b962ebbf8beab8c8593f645797506c43
comment:12 Changed 4 months ago by ncohen
That was all ? Cool ! :-D
I have got no working install of Sage right now but it should be reviewed tonight, or tomorrow morning.
Nathann
comment:13 Changed 4 months ago by git
- Commit changed from a8766b07b962ebbf8beab8c8593f645797506c43 to 0f36ea8c8330288a2da5c0eb135881fc7ed5612c
Branch pushed to git repo; I updated commit sha1. New commits:
0f36ea8 | 15545: improve fix |
comment:14 Changed 4 months ago by rws
Sadly, I had to change the 2-character into a 3-character fix because of style issues.
comment:15 Changed 4 months ago by ncohen
- Reviewers set to Nathann Cohen
- Status changed from needs_review to positive_review
Okayyyyyyy good !
Nathann
comment:16 Changed 4 months ago by vbraun
- Branch changed from u/rws/typeerror_in_matching_polynomial to 0f36ea8c8330288a2da5c0eb135881fc7ed5612c
- Resolution set to fixed
- Status changed from positive_review to closed
If not a fix, I would expect "Maybe you should relabel the graph." from the function.