Opened 10 months ago

Closed 5 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 9 months ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:2 Changed 6 months ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:3 Changed 5 months ago by rws

  • Priority changed from minor to major

If not a fix, I would expect "Maybe you should relabel the graph." from the function.

comment:4 follow-up: Changed 5 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: Changed 5 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: Changed 5 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 5 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 5 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 5 months ago by rws

  • Branch set to u/rws/typeerror_in_matching_polynomial

comment:10 Changed 5 months ago by rws

  • Authors set to Ralf Stephan
  • 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:

e295da315545: do not use Cython var for unrelated purposes

comment:11 Changed 5 months ago by git

  • Commit changed from e295da30f2ccbd64b5fdbed4571333ad7aeb816b to a8766b07b962ebbf8beab8c8593f645797506c43

Branch pushed to git repo; I updated commit sha1. New commits:

8d9064815545: doctest
a8766b0Merge branch 'develop' into t/15545/typeerror_in_matching_polynomial

comment:12 Changed 5 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 5 months ago by git

  • Commit changed from a8766b07b962ebbf8beab8c8593f645797506c43 to 0f36ea8c8330288a2da5c0eb135881fc7ed5612c

Branch pushed to git repo; I updated commit sha1. New commits:

0f36ea815545: improve fix

comment:14 Changed 5 months ago by rws

Sadly, I had to change the 2-character into a 3-character fix because of style issues.

comment:15 Changed 5 months ago by ncohen

  • Reviewers set to Nathann Cohen
  • Status changed from needs_review to positive_review

Okayyyyyyy good !

Nathann

comment:16 Changed 5 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
Note: See TracTickets for help on using tickets.