-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow cimport for quicktions #5
base: master
Are you sure you want to change the base?
Changes from 23 commits
479ad2d
4ab0b49
2a10e91
0a6e8e2
cee0127
b59f3ba
278516c
18f7389
46daa88
3be2c4e
13b03b5
32fe9ea
53e1250
08ac125
f628e89
6de8ea4
5cb185a
19ea21c
b7d99f4
f8894e9
fad41af
dde7654
efcca15
0536e1e
5a477cc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,8 +9,8 @@ __pycache__ | |
|
||
/build | ||
/dist | ||
src/*.c | ||
src/*.html | ||
src/quicktions/*.c | ||
src/quicktions/*.html | ||
MANIFEST | ||
|
||
.tox | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
include MANIFEST.in LICENSE *.rst | ||
include setup.py *.yml tox.ini *.cmd *.txt | ||
include test_fractions.py | ||
recursive-include src *.py *.pyx *.pxd *.c *.html | ||
recursive-include benchmark *.py telco-bench.b |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
from .quicktions import Fraction, _gcd | ||
from quicktions.quicktions cimport Fraction, _gcd |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
from .quicktions import Fraction, _gcd | ||
from .quicktions import __doc__, __version__, __all__ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
# cython: language_level=3str | ||
## cython: profile=True | ||
|
||
cdef extern from *: | ||
""" | ||
#if PY_VERSION_HEX < 0x030500F0 || !CYTHON_COMPILING_IN_CPYTHON | ||
#define _PyLong_GCD(a, b) (NULL) | ||
#endif | ||
""" | ||
# CPython 3.5+ has a fast PyLong GCD implementation that we can use. | ||
int PY_VERSION_HEX | ||
int IS_CPYTHON "CYTHON_COMPILING_IN_CPYTHON" | ||
_PyLong_GCD(a, b) | ||
|
||
ctypedef unsigned long long ullong | ||
ctypedef unsigned long ulong | ||
ctypedef unsigned int uint | ||
|
||
ctypedef fused cunumber: | ||
ullong | ||
ulong | ||
uint | ||
|
||
cpdef _gcd(a, b) | ||
cdef ullong _abs(long long x) | ||
cdef cunumber _igcd(cunumber a, cunumber b) | ||
cdef cunumber _ibgcd(cunumber a, cunumber b) | ||
cdef _py_gcd(ullong a, ullong b) | ||
cdef _gcd_fallback(a, b) | ||
|
||
cdef class Fraction: | ||
cdef _numerator | ||
cdef _denominator | ||
cdef Py_hash_t _hash | ||
scoder marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
cpdef limit_denominator(self, max_denominator=*) | ||
cpdef conjugate(self) | ||
cdef _eq(a, b) | ||
cdef _richcmp(self, other, int op) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if it's possible to leave out these two ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All methods and attributes in extension types must be declared in the pxd. It doesn't alter the behavior outside of Cython, but it does allow the accelerated use of those methods by other Cython modules (whether directly or indirectly). |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -70,16 +70,6 @@ cdef pow10(Py_ssize_t i): | |
|
||
# Half-private GCD implementation. | ||
|
||
cdef extern from *: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please keep this in here. It's strictly an internal implementation detail of the module. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am trying to remember if this being in the pyx caused compilation errors or not, but will test |
||
""" | ||
#if PY_VERSION_HEX < 0x030500F0 || !CYTHON_COMPILING_IN_CPYTHON | ||
#define _PyLong_GCD(a, b) (NULL) | ||
#endif | ||
""" | ||
# CPython 3.5+ has a fast PyLong GCD implementation that we can use. | ||
int PY_VERSION_HEX | ||
int IS_CPYTHON "CYTHON_COMPILING_IN_CPYTHON" | ||
_PyLong_GCD(a, b) | ||
|
||
|
||
cpdef _gcd(a, b): | ||
|
@@ -94,14 +84,6 @@ cpdef _gcd(a, b): | |
return _PyLong_GCD(a, b) | ||
|
||
|
||
ctypedef unsigned long long ullong | ||
ctypedef unsigned long ulong | ||
ctypedef unsigned int uint | ||
|
||
ctypedef fused cunumber: | ||
ullong | ||
ulong | ||
uint | ||
|
||
|
||
cdef ullong _abs(long long x): | ||
|
@@ -244,9 +226,6 @@ cdef class Fraction: | |
Fraction(147, 100) | ||
|
||
""" | ||
cdef _numerator | ||
cdef _denominator | ||
cdef Py_hash_t _hash | ||
|
||
def __cinit__(self, numerator=0, denominator=None, *, bint _normalize=True): | ||
cdef Fraction value | ||
|
@@ -388,7 +367,7 @@ cdef class Fraction: | |
else: | ||
return cls(digits, pow10(-exp)) | ||
|
||
def limit_denominator(self, max_denominator=1000000): | ||
cpdef limit_denominator(self, max_denominator=1000000): | ||
"""Closest Fraction to self with denominator at most max_denominator. | ||
|
||
>>> Fraction('3.141592653589793').limit_denominator(10) | ||
|
@@ -607,7 +586,7 @@ cdef class Fraction: | |
"Real numbers have no imaginary component." | ||
return 0 | ||
|
||
def conjugate(self): | ||
cpdef conjugate(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you really think this method is worth There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably not since there's no typing or any other cython-related code there. It was mainly to reduce context switches between Python and Cython calls, but there's really not much going on in that method. |
||
"""Conjugate is a no-op for Reals.""" | ||
return +self | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ | |
|
||
from __future__ import division | ||
|
||
import os | ||
from decimal import Decimal | ||
import math | ||
import numbers | ||
|
@@ -796,11 +797,50 @@ def test_pi_digits(self): | |
self.assertEqual(ff.numerator, qf.numerator) | ||
self.assertEqual(ff.denominator, qf.denominator) | ||
|
||
class CImportTest(unittest.TestCase): | ||
scoder marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
def setUp(self): | ||
self.build_test_module() | ||
scoder marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
def tearDown(self): | ||
self.remove_test_module() | ||
|
||
def build_test_module(self): | ||
self.module_code = '\n'.join([ | ||
scoder marked this conversation as resolved.
Show resolved
Hide resolved
|
||
'# cython: language_level=3str', | ||
'from quicktions cimport Fraction', | ||
'def get_fraction():', | ||
' return Fraction(1, 2)', | ||
]) | ||
self.base_path = os.path.abspath(os.path.dirname(__file__)) | ||
self.module_name = 'quicktions_importtest' | ||
self.module_filename = os.path.join(self.base_path, '.'.join([self.module_name, 'pyx'])) | ||
with open(self.module_filename, 'w') as f: | ||
f.write(self.module_code) | ||
|
||
def remove_test_module(self): | ||
for fn in os.listdir(self.base_path): | ||
if not fn.startswith(self.module_name): | ||
continue | ||
os.remove(os.path.join(self.base_path, fn)) | ||
|
||
def test_cimport(self): | ||
self.build_test_module() | ||
import pyximport | ||
self.py_importer, self.pyx_importer = pyximport.install(inplace=True, language_level=3) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please don't use pyximport for the tests. It's really outdated. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did look into that as it would be much cleaner, but from what I've tested, import cython
r = cython.inline('from quicktions cimport Fraction;return Fraction(a,b)', a=1,b=2) Raises an exception:
I agree that there should be a better way though |
||
|
||
from quicktions_importtest import get_fraction | ||
|
||
self.assertEqual(get_fraction(), F(1,2)) | ||
|
||
pyximport.uninstall(self.py_importer, self.pyx_importer) | ||
|
||
|
||
def test_main(): | ||
suite = unittest.TestSuite() | ||
suite.addTest(unittest.makeSuite(GcdTest)) | ||
suite.addTest(unittest.makeSuite(FractionTest)) | ||
suite.addTest(unittest.makeSuite(CImportTest)) | ||
import doctest | ||
suite.addTest(doctest.DocTestSuite('quicktions')) | ||
return suite | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,7 @@ platform = | |
linux: linux | ||
darwin: darwin | ||
passenv = * | ||
commands = coverage run --parallel-mode -m pytest src/test_fractions.py --capture=no --strict {posargs} | ||
commands = coverage run --parallel-mode -m pytest test_fractions.py --capture=no --strict {posargs} | ||
coverage combine | ||
coverage report -m --include=src/test_fractions.py | ||
coverage report -m --include=test_fractions.py | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should include the new test file. |
||
{windows,linux}: codecov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think any of these should be exported. They are really just implementation details and subject to change at any time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly to above this may not be necessary. I'll do some testing