Opened 3 years ago
Closed 3 years ago
#15545 closed defect (fixed)
TypeError in matching_polynomial
Reported by:  rlm  Owned by:  

Priority:  major  Milestone:  sage6.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) <ipythoninput68174221329fce> in <module>() > 1 G.matching_polynomial() /Users/miller/sage5.10/local/lib/python2.7/sitepackages/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 3 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:2 Changed 3 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:3 Changed 3 years ago by
 Priority changed from minor to major
comment:4 followup: ↓ 8 Changed 3 years ago by
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 followup: ↓ 6 Changed 3 years ago by
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 ; followup: ↓ 7 Changed 3 years ago by
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 3 years ago by
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 3 years ago by
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 3 years ago by
 Branch set to u/rws/typeerror_in_matching_polynomial
comment:10 Changed 3 years ago by
 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 3 years ago by
 Commit changed from e295da30f2ccbd64b5fdbed4571333ad7aeb816b to a8766b07b962ebbf8beab8c8593f645797506c43
comment:12 Changed 3 years ago by
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 3 years ago by
 Commit changed from a8766b07b962ebbf8beab8c8593f645797506c43 to 0f36ea8c8330288a2da5c0eb135881fc7ed5612c
Branch pushed to git repo; I updated commit sha1. New commits:
0f36ea8  15545: improve fix

comment:14 Changed 3 years ago by
Sadly, I had to change the 2character into a 3character fix because of style issues.
comment:15 Changed 3 years ago by
 Reviewers set to Nathann Cohen
 Status changed from needs_review to positive_review
Okayyyyyyy good !
Nathann
comment:16 Changed 3 years ago by
 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.