Opened 10 years ago

Closed 9 years ago

#11614 closed defect (fixed)

Make Cython libcpp usable

Reported by: vbraun Owned by: jason
Priority: major Milestone: sage-5.0
Component: misc Keywords:
Cc: jdemeyer, leif Merged in: sage-5.0.beta2
Authors: Volker Braun Reviewers: Jeroen Demeyer, John Perry
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

Cython's libcpp allows use of C++ container classes (aka STL), but using it from Sage is still challenging. A simple from libcpp.vector cimport vector (http://docs.cython.org/src/userguide/wrapping_CPlusPlus.html#standard-library) does not work in a Sage extension class because setup.py will not find some of the dependencies.

This ticket aims to fix this and add an example to the Sage libary documenting its use.

Attachments (1)

trac_11614_stl_vectors.patch (6.0 KB) - added by jdemeyer 9 years ago.
Rebased to sage-5.0.beta1

Download all attachments as: .zip

Change History (19)

comment:1 Changed 10 years ago by vbraun

  • Authors set to Volker Braun
  • Cc jdemeyer added
  • Status changed from new to needs_review

comment:2 Changed 10 years ago by leif

  • Cc leif added

comment:3 Changed 10 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to needs_work

My most important question about this is: what is the purpose of this code? It looks like it is meant only as an example, since it has basically no features. I'm not sure whether we want to put code into Sage which is only meant as example. There is supposed to be examples/ for this, but I realize that this is totally unmaintained (and therefore scheduled for removal).

Then some random thoughts:

1) Why

cdef vector[int] *data

and not

cdef vector[int] data

Considering that a std::vector<int> is essentially a pointer to an array, there is no need to declare a pointer to a pointer.

2) You should make the copyright statement consistent with the template at http://sagemath.org/doc/developer/conventions.html#headings-of-sage-library-code-files

3) You should add an example of __getitem__ failing.

comment:4 Changed 10 years ago by vbraun

  • Status changed from needs_work to needs_review

As for why, right now Cython allows us to use stl classes but our setup.py is broken. This ticket fixes it and adds a testcase for future reference. I was very surprised that this issue was not reported before. My guess is that nobody figured out how to use it, which is why I want to have an example in the Sage library at least until it STL containers are used more often so that there are examples elsewhere.

1) Thats not allowed by Cython:

Error compiling Cython file:
------------------------------------------------------------
...

        sage: from sage.tests.stl_vector import stl_int_vector
        sage: v = stl_int_vector()
    """

    cdef vector[int] data
                    ^
------------------------------------------------------------

sage/tests/stl_vector.pyx:40:21: C++ classes not allowed as members of an extension type, use a pointer or reference instead

2,3) Done

comment:5 Changed 9 years ago by john_perry

I can't get this to work on alpha0 or alpha4; I get, for example,

Error converting Pyrex file to C:
------------------------------------------------------------
...
            sage: TestSuite(v)
            Test suite for vector<int>:
             data[0] = 123
             data[1] = 456
        """
        self.data = new vector[int]()
                       ^
------------------------------------------------------------

/Users/user/.sage/temp/Users_MacBook_Pro.local/5523/spyx/_Users_user__sage_temp_Users_MacBook_Pro_local_5523_tmp_0_spyx/_Users_user__sage_temp_Users_MacBook_Pro_local_5523_tmp_0_spyx_0.pyx:62:24: Operation only allowed in c++

I find that odd, considering it's supposed to be C++. Is there a minimum alpha revision for which this works?

comment:6 Changed 9 years ago by vbraun

Sorry, at one point I dropped the entry in module_list.py so the C extension module did not get compiled. The updated patch fixes this and gives a minimal example of how to use std::string. It works fine with sage-4.8alpha6. The alpha0 might be too old (not sure), but alpha4 should be fine though I haven't tested it.

comment:7 Changed 9 years ago by vbraun

PS: I just checked and it seems we upgrade to cython-0.15.1 in sage4.8alpha2 so anything past that should work just as alpha6.

comment:8 Changed 9 years ago by john_perry

I'm getting the same doctest errors, plus more, what with the addition to the tests.pyx file of string examples. This is on alpha4.

I'm not likely to update to the latest alpha over the weekend; if it still needs a review next week, I'll download a fresh copy of the latest alpha then, and see if it will work.

comment:9 Changed 9 years ago by vbraun

I've tried out the patch on bsd.math on top of sage-4.8.alpha3 and it works for me. Did you forget to rebuild the Sage library (sage -b)? Please post the whole output.

comment:10 Changed 9 years ago by john_perry

Did you forget to rebuild the Sage library (sage -b)?

C'mon, I'm not that dumb! X-D But just to be sure, I tried building & testing again, and I encountered the same errors.

I do think the following output from building is pertinent (slightly edited for readability):

setup.py:693: UserWarning: could not find dependency <string> included in
/Applications/sage-4.8.alpha4/local/lib/python/site-packages/Cython/Includes/libcpp/string.pxd.
I will assume it is a system C/C++ header.
  warnings.warn(msg+' I will assume it is a system C/C++ header.')
setup.py:693: UserWarning: could not find dependency <vector> included in
/Applications/sage-4.8.alpha4/local/lib/python/site-packages/Cython/Includes/libcpp/vector.pxd.
I will assume it is a system C/C++ header.
  warnings.warn(msg+' I will assume it is a system C/C++ header.')

The file referenced is definitely there, and includes a declaration of C headers. If I inspect the file, I see a declaration of a cppclass from <string> that should cover it. Yet, when I run the doctests, I get boatloads of errors of this sort:

Detected SAGE64 flag
Building Sage on OS X in 64-bit mode
sage -t  "sage/tests/stl_vector.pyx"                        
Traceback (most recent call last):
  File "/Users/user/.sage//tmp/stl_vector.py", line 18, in <module>
    cython(open('sage/tests/stl_vector.pyx').read())
  File "cython_c.pyx", line 32, in sage.misc.cython_c.cython (sage/misc/cython_c.c:645)
  File "/Applications/Sage-4.6.2-OSX-64bit-10.6.app/Contents/Resources/sage/local
/lib/python/site-packages/sage/server/support.py", line 469, in cython_import_all
    create_local_c_file=create_local_c_file)
  File "/Applications/Sage-4.6.2-OSX-64bit-10.6.app/Contents/Resources/sage/local
/lib/python/site-packages/sage/server/support.py", line 446, in cython_import
    create_local_c_file=create_local_c_file)
  File "/Applications/Sage-4.6.2-OSX-64bit-10.6.app/Contents/Resources/sage/local
/lib/python/site-packages/sage/misc/cython.py", line 367, in cython
    raise RuntimeError, "Error converting %s to C:\n%s\n%s"%(filename, log, err)
RuntimeError: Error converting /Users/user/.sage//temp/Users_MacBook_Pro.local/
9281//tmp_0.spyx to C:


Error converting Pyrex file to C:
------------------------------------------------------------
...

from sage.structure.sage_object cimport SageObject
from sage.rings.integer cimport Integer
from sage.libs.gmp.mpz cimport mpz_add_ui
from libcpp.vector cimport vector
from libcpp.string cimport string
^
------------------------------------------------------------

/Users/user/.sage/temp/Users_MacBook_Pro.local/9281/spyx
/_Users_user__sage_temp_Users_MacBook_Pro_local_9281_tmp_0_spyx
/_Users_user__sage_temp_Users_MacBook_Pro_local_9281_tmp_0_spyx_0.pyx:37:0:
'libcpp.string.pxd' not found


Error converting Pyrex file to C:
------------------------------------------------------------
...

from sage.structure.sage_object cimport SageObject
from sage.rings.integer cimport Integer
from sage.libs.gmp.mpz cimport mpz_add_ui
from libcpp.vector cimport vector
from libcpp.string cimport string
^
------------------------------------------------------------

/Users/user/.sage/temp/Users_MacBook_Pro.local/9281/spyx
/_Users_user__sage_temp_Users_MacBook_Pro_local_9281_tmp_0_spyx
/_Users_user__sage_temp_Users_MacBook_Pro_local_9281_tmp_0_spyx_0.pyx:37:0: 
'string.pxd' not found


Error converting Pyrex file to C:
------------------------------------------------------------
...

from sage.structure.sage_object cimport SageObject
from sage.rings.integer cimport Integer
from sage.libs.gmp.mpz cimport mpz_add_ui
from libcpp.vector cimport vector
from libcpp.string cimport string
                          ^
------------------------------------------------------------

/Users/user/.sage/temp/Users_MacBook_Pro.local/9281/spyx
/_Users_user__sage_temp_Users_MacBook_Pro_local_9281_tmp_0_spyx
/_Users_user__sage_temp_Users_MacBook_Pro_local_9281_tmp_0_spyx_0.pyx:37:27: Name 
'string' not declared in module 'libcpp.string'


Error converting Pyrex file to C:
------------------------------------------------------------
...
        sage: from sage.tests.stl_vector import stl_int_vector
        sage: v = stl_int_vector()
    """

    cdef vector[int] *data
    cdef string *name
        ^
------------------------------------------------------------

/Users/user/.sage/temp/Users_MacBook_Pro.local/9281/spyx
/_Users_user__sage_temp_Users_MacBook_Pro_local_9281_tmp_0_spyx
/_Users_user__sage_temp_Users_MacBook_Pro_local_9281_tmp_0_spyx_0.pyx:50:9: 'string' 
is not a type identifier


Error converting Pyrex file to C:
------------------------------------------------------------
...
            Test suite for A vector of integers
            vector<int>:
             data[0] = 123
             data[1] = 456
        """
        self.data = new vector[int]()
                       ^
------------------------------------------------------------

/Users/user/.sage/temp/Users_MacBook_Pro.local/9281/spyx
/_Users_user__sage_temp_Users_MacBook_Pro_local_9281_tmp_0_spyx
/_Users_user__sage_temp_Users_MacBook_Pro_local_9281_tmp_0_spyx_0.pyx:66:24: Operation 
only allowed in c++

and more just like it.

I suppose it's possible that this installation of sage is simply pooched, though that would seem very, very weird; other patches go in without a problem. It's a MacBook? Pro running 10.6.8. On my Ubuntu box running sage 4.8.rc0, the installation passes.

I will run the full battery of doctests on the Ubuntu box, and also try a fresh install of 4.8.rc0 on the MacBook? Pro. If it all works out, I'll give it a positive review.

comment:11 follow-up: Changed 9 years ago by vbraun

File "/Applications/Sage-4.6.2-OSX-64bit-10.6.app/Contents/Resources/sage/local
/lib/python/site-packages/sage/misc/cython.py", line 367, in cython

Note the path, you are running cython from sage-4.6.2. I'm pretty sure that this ancient version does not have C++ support. It seems like you use the wrong sage (perhaps wrong PATH?) to run the doctests.

comment:12 in reply to: ↑ 11 Changed 9 years ago by john_perry

Replying to vbraun:

Note the path, you are running cython from sage-4.6.2. I'm pretty sure that this ancient version does not have C++ support. It seems like you use the wrong sage (perhaps wrong PATH?) to run the doctests.

That's really weird. I am in the correct path, and type

../../sage -b

So I don't know why it's doing that. I'll look at it later.

comment:13 Changed 9 years ago by john_perry

In any case, it works on 4.8.rc0 on the MacBook. Now I'm just waiting for Ubuntu's tests to finish.

comment:14 Changed 9 years ago by john_perry

Sorry, I have to correct myself: that '''was''' 4.8.alpha4. I really have no idea why it didn't work before. Perhaps I typed `sage` instead of `../../sage`...? (as you say, the path.) Oh, well.

comment:15 Changed 9 years ago by john_perry

  • Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, John Perry
  • Status changed from needs_review to positive_review

All tests passed! While I'm not an expert in Cython, what I see of the code makes sense, too. (& thanks for illustrating the use of `string`.) I'm giving it a positive review, but there are two points I want to ask about (& which perhaps should prevent positive review, but I'm not sure):

  1. You should probably change this in the comments:
Compare whith !``other!``.

That should be "with" not "whith". :-)

  1. The format opening the file is not quite in conformance with the format Jeroen requested. There is no list of authors, and the copyright notice is not quite the same. It's close enough that I'm okay with it, & I've seen other files that don't follow this pattern. Nevertheless, I should point it out.

comment:16 Changed 9 years ago by vbraun

The updated patch fixes the typo and extends the header to match the template.

comment:17 Changed 9 years ago by jdemeyer

  • Milestone changed from sage-4.8 to sage-5.0

Changed 9 years ago by jdemeyer

Rebased to sage-5.0.beta1

comment:18 Changed 9 years ago by jdemeyer

  • Merged in set to sage-5.0.beta2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.