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: |
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)
Change History (19)
comment:1 Changed 10 years ago by
- Cc jdemeyer added
- Status changed from new to needs_review
comment:2 Changed 10 years ago by
- Cc leif added
comment:3 Changed 10 years ago by
- Reviewers set to Jeroen Demeyer
- Status changed from needs_review to needs_work
comment:4 Changed 10 years ago by
- 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
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
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
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
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
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
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: ↓ 12 Changed 9 years ago by
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
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
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
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
- 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):
- You should probably change this in the comments:
Compare whith !``other!``.
That should be "with" not "whith". :-)
- 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
The updated patch fixes the typo and extends the header to match the template.
comment:17 Changed 9 years ago by
- Milestone changed from sage-4.8 to sage-5.0
comment:18 Changed 9 years ago by
- Merged in set to sage-5.0.beta2
- Resolution set to fixed
- Status changed from positive_review to closed
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
and not
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.