Opened 8 years ago

Closed 5 years ago

#11945 closed defect (fixed)

Throw exception instead of printing error in c_graph.pyx

Reported by: kini Owned by: jason, ncohen, rlm
Priority: major Milestone: sage-6.4
Component: graph theory Keywords: cython exception cpdef print c_graph
Cc: rlm Merged in:
Authors: Jeroen Demeyer Reviewers: Nathann Cohen
Report Upstream: N/A Work issues:
Branch: adf8d74 (Commits) Commit: adf8d747e145b9374fe9b5bebca4f2379a50fd99
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

The following text currently occurs in $SAGE_ROOT/devel/sage/sage/graphs/base/c_graph.pyx:

        # The following is due to a hard to reproduce bug in Cython where except,
        # cpdef, and classes don't play well together:
        print "Not Implemented!"
        # raise NotImplementedError() ... results in:
        # Exception exceptions.NotImplementedError: NotImplementedError() in 'sage.graphs.base.c_graph.CGraph.has_arc' ignored
        # False

There is no Cython bug, it's just a matter of properly declaring the except value in the .pxd file (not just the .pyx file). The attached patch further adds except values to some other functions where needed.

Change History (22)

comment:1 Changed 8 years ago by kini

  • Cc rlm added

CCing rlm as he is the most likely to know more about this.

By the way, the "exception ignored" message appears to be showing up on stderr rather than stdout.

comment:2 Changed 8 years ago by jdemeyer

  • Milestone sage-4.7.3 deleted

Milestone sage-4.7.3 deleted

comment:3 Changed 5 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Description modified (diff)
  • Milestone set to sage-6.4

comment:4 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:5 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:6 Changed 5 years ago by jdemeyer

  • Branch set to u/jdemeyer/ticket/11945
  • Modified changed from 09/26/14 13:18:37 to 09/26/14 13:18:37

comment:7 Changed 5 years ago by jdemeyer

  • Commit set to 2de64d45efaa70525932f6d086aef3afbef31cbe
  • Status changed from new to needs_review

New commits:

2de64d4Add proper "except" error return values

comment:8 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:9 follow-up: Changed 5 years ago by ncohen

  • Status changed from needs_review to needs_work

O_o

I had not noticed this patch. Could you rebase it ? There is a merge conflict.

Also it may be in conflict with a needs_review patch I just wrote (#17163), but nothing complicated in sight.

Nathann

comment:10 follow-up: Changed 5 years ago by ncohen

O_o

Why would you do that ? O_o

-        raise ValueError("Thou shalt not add a vertex to an immutable graph")
+        self.add_vertex_unsafe(k)

comment:11 in reply to: ↑ 10 ; follow-up: Changed 5 years ago by jdemeyer

Replying to ncohen:

Why would you do that ? O_o

-        raise ValueError("Thou shalt not add a vertex to an immutable graph")
+        self.add_vertex_unsafe(k)

Throwing the same exception in 2 different places seemed a bit stupid. And it allows to actually test the add_vertex_unsafe method.

comment:12 in reply to: ↑ 9 Changed 5 years ago by jdemeyer

Replying to ncohen:

O_o

I had not noticed this patch.

If you're interested, there is a similar patch needing review at #16233.

comment:13 in reply to: ↑ 11 ; follow-up: Changed 5 years ago by ncohen

Throwing the same exception in 2 different places seemed a bit stupid. And it allows to actually test the add_vertex_unsafe method.

Come on, leave it as it is. What is the point ? Keep it simple: this thing is not implemented because the class is immutable, let it return an exception.

Nathann

comment:14 in reply to: ↑ 13 Changed 5 years ago by jdemeyer

Replying to ncohen:

Come on, leave it as it is. What is the point ?

Like I said: it allows to actually test the add_vertex_unsafe method, which is otherwise untested.

Keep it simple: this thing is not implemented because the class is immutable, let it return an exception.

My code is equally simple and does exactly the same thing, so why not?

comment:15 Changed 5 years ago by git

  • Commit changed from 2de64d45efaa70525932f6d086aef3afbef31cbe to adf8d747e145b9374fe9b5bebca4f2379a50fd99

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

9405ee9Merge remote-tracking branch 'origin/develop' into ticket/11945
adf8d74Fix Cython warnings

comment:16 Changed 5 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:17 follow-ups: Changed 5 years ago by ncohen

How does "except -1" work when the return type is bint ?

Also, won't this cause that some calls which are currently C calls will become Python calls ?

 cdef class StaticSparseCGraph(CGraph):
     cdef short_digraph g
     cdef short_digraph g_rev
     cdef bint _directed
 
-    cpdef bint has_vertex(self, int n)
-    cdef int add_vertex_unsafe(self, int k)
-    cdef int del_vertex_unsafe(self, int v)
-    cpdef list verts(self)
-    cdef int has_arc_unsafe(self, int u, int v)
-    cpdef bint has_arc(self, int u, int v)
-    cdef int out_neighbors_unsafe(self, int u, int *neighbors, int size) except? -2
-    cpdef list out_neighbors(self, int u)
     cpdef int out_degree(self, int u) except -1
-    cdef int in_neighbors_unsafe(self, int u, int *neighbors, int size) except? -2
-    cpdef list in_neighbors(self, int u)
     cpdef int in_degree(self, int u) except -1

Nathann

comment:18 in reply to: ↑ 17 Changed 5 years ago by jdemeyer

Replying to ncohen:

How does "except -1" work when the return type is bint ?

The actual type is a C int. The return values mean the following:

  • 0 => False
  • 1 => True
  • -1 => exception

comment:19 in reply to: ↑ 17 Changed 5 years ago by jdemeyer

Replying to ncohen:

How does "except -1" work when the return type is bint ?

Also, won't this cause that some calls which are currently C calls will become Python calls ?

No, there are already declared in the child class CGraph, no point to repeat them.

comment:20 Changed 5 years ago by ncohen

Okay, then as soon as I am done building the latest beta I will run all tests on your patch, should be done soon.

Nathann

comment:21 Changed 5 years ago by ncohen

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

comment:22 Changed 5 years ago by vbraun

  • Branch changed from u/jdemeyer/ticket/11945 to adf8d747e145b9374fe9b5bebca4f2379a50fd99
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.