Skip to content
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

Add a Pythran backend for Numpy operation #1607

Merged
merged 5 commits into from Jun 27, 2017

Conversation

aguinet
Copy link
Contributor

@aguinet aguinet commented Feb 15, 2017

I've been working for quite some time on the usage of Pythran as a backend for
the Numpy operations that Cython can generate. The associated PR on github can
be found here: . This work has been sponsored by the OpenDreamKit project
(https://github.com/OpenDreamKit/OpenDreamKit/).

First of all, the Pythran project
(https://github.com/serge-sans-paille/pythran) is a (subset of) Python to C++
compiler, that aims at optimizing "scientific" Python code. It also provides a
full C++ implementation of a major set of the Numpy API. Some of the advantage
of this implementation is that it supports expression templates and SIMD
instructions (partially thanks to Boost.SIMD [1]).

One of the limitation of the current Numpy support of Cython is that it relies
on the original Numpy Python module for a lot of computations. The overall idea
is to replace these calls by the Numpy implementation provided within the
Pythran project.

I'll discuss in this mail the various choices that have been made, why and some
implementation details. Then we'll also show some benchmark to see the
potential improvements, which is the point of all this in the end :)

Pythran limitations

The Pythran Numpy implementation has some limitations:

  • array "views" are not supported. That means that arrays must be stored in
    contiguous memory. Fortran and C-style format are supported.
  • the endianness of the integers must be the same that the one of the targeted
    architecture (note that Cython has the same limitation)

That's why we did two things:

  • the usage of the Pythran backend needs to be explicitly asked by the user by
    providing the --np-pythran flag to the Cython compiler, or by using the
    "np_pythran" flag to the cythonize call (for distutils)
  • in function arguments, Numpy buffers are replaced by fused types to be able
    to fall back in case of unsupported buffers. More on this below.

Implementation choices and details within Cython

a) PythranExpr

We defined a new type in PyrexTypes.py, which defines a Pythran buffer or
expression. A Pythran expression is associated to a Pythran expression
template, whose C++ type can be something like "decltype(a+b)". We thus compose
every expression/function call like this, which allows us to use Pythran's
expression template mechanism.

We also choose to let the C++ compiler deduced the final type of every
expression, and emit errors if something goes wrong. This choice allows not to
have to rewrite in Python all the (potentially implicit) conversion rules that
can apply in a C/C++ program, which could be error prone. The disadvantage is
that it may generate not that trivial error messages for the end-user.

b) Fused types for function arguments

As Pythran has some limitations about the Numpy buffers it can support, we
chose to replace Numpy buffer arguments by a fused type that can be either a
Pythran buffer or the original Numpy buffer. The decision is made to use one
type or another according to the limitations described above.

This allows a fallback to the original Cython implementation in case of an
unsupported buffer type.

Tests

A flag has been added to the runtests.py script. If provided with a path to a
Pythran installation, it will run the C++ tests in "Pythran" mode. This allows
to reuse the whole test suite of Cython.

Benchmark

The whole idea of this is to get better performances.

Here is a simple benchmark of what this mode can achieve, using this cython code:

def sqrt_sum(numpy.ndarray[numpy.float_t, ndim=1] a, numpy.ndarray[numpy.float_t, ndim=1] b):
    return numpy.sqrt(numpy.sqrt(a*a+b*b))

On my computer (Core i7-6700HQ), this gives there results, with an array of
100000000 32-bit floats as input:

  • for the classical Cython version: 960ms
  • for the Cython version using the Pythran backend: 761ms
  • for the Cython version using the Pythran backend using SIMD instructions: 243ms

which makes a speedup of ~3.9x using SIMD instructions.

Documentation

I put an example of how to use this with distutils in the documentation. It
could be put elsewhere if needed, or formatted differently.

@scoder
Copy link
Contributor

scoder commented Feb 19, 2017

Nice! Is this actually limited to numpy or could it also support memoryviews?

Copy link
Contributor

@scoder scoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - from a quick read through the code I can only see some minor issues here and there. Haven't tried it, though.

@@ -6561,6 +6711,11 @@ def analyse_as_python_attribute(self, env, obj_type=None, immutable_obj=False):
self.member = self.attribute
self.type = py_object_type
self.is_py_attr = 1
# Check for numpy calls
# TODO: this is a very weak test, but how to make it better?
if self.obj.is_name and self.obj.name == "numpy":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We track (c)imports of the special "cython" module in a tree transform. We could do the same for the numpy module, that would make this test safer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One first improvement here: ed5f933

if self.is_py_operation():
if self.is_pythran_operation(env):
self.type = self.result_type(self.operand1.type,
self.operand2.type,env)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a space after the "," (as in a couple of other places).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e3fb29c

u"""
def __is_little_endian():
cdef int endian_detector = 1
return (<char*>&endian_detector)[0] != 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually based this code on code that already exists in cython :) I'll take a look at what this stack overflow thread propose!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e3fb29c

@@ -569,6 +570,10 @@ def __init__(self, defaults=None, **kw):
if options['cache'] is True:
options['cache'] = os.path.expanduser("~/.cycache")

if options['np_pythran'] and not options['cplus']:
import warnings
warnings.warn("C++ mode forced when in Pythran mode!")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's good to warn about this.

{
o.~T();
}
''')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static code like this should go into an external utility code file. I'd propose CppSupport.cpp in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e3fb29c

return "decltype(pythonic::__builtin__::None)"
if Ty.is_numeric:
return Ty.sign_and_name()
raise ValueError("unsupported pythran type %s (%s)" % (str(Ty), str(type(Ty))))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to call str() here, that's what "%s" does.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e3fb29c


def pythran_unaryop_type(op, type_):
return "decltype(%sstd::declval<%s>())" % \
(op, pythran_type(type_))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor code style nit: you can avoid the ugly trailing line continuation by starting the tuple in the first line and breaking it after the opening parenthesis.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e3fb29c


def is_type(type_, types):
for attr in types:
if hasattr(type_, attr) and getattr(type_, attr):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3-args getattr() is enough here, no need for hasattr().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e3fb29c

#if hasattr(op,"entry") and hasattr(op.entry, "pythran_buf") and op.entry.pythran_buf is not None:
# return op.entry.pythran_buf.cname
op_type = op.type
if is_type(op_type,["is_pythran_expr","is_int","is_numeric","is_float","is_complex"]):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As before, spaces make this list more readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e3fb29c


setup(
name = "My hello app",
ext_modules = cythonize('hello_pythran.pyx', np_pythran=True)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather have people use a directive here (i.e. put # cython: np_pythran=True at the top of your file), rather than a cythonize() option.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to add a cython directive, but the main issue I see is that we need toforce the language to C++. This needs to be done at a module level, and thus doesn't seem compatible with a Cython compiler directive, that can be applied for instance at a function level.

I tried to add a distutils "settings", but they are forwarded straight to the disutils.Extension object constructor, thought we can "override" this by some not that nice hacks.

Do you have other idea to do this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could live with having it fail hard if it finds a pythran directive in non-C++ mode. If it's enabled at the module level (comment at top of file), switching on C++ mode automatically seems ok, but for in-module usage, failing to compile and explaining how to fix it is just fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started to add a np_pythran compiler attribute in ff71312 . I "just" need to get this information before compoilation to add the include directory to the Pytran module. If I understand correctly, it seems that, for now, "only" distuils directives are taken into account at the compilation stage. If I understand correctly, I need to add a "parsing" the same way it is done for distutils directives.

Do I miss something ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be ok with adding a special case in the DistutilsInfo class that looks for np_pythran in addition to distutils: and does all the build setup changes right there. Basically, if you use in-module configuration for distutils (and np_pythran), then you need to call cythonize(), so cythonize() can rely on DistutilsInfo doing its job, including the setup for np_pythran. If cythonize() is not used, then the in-module config for np_pythran is simply not available (and can thus be made an error) and instead, the Extension instance must be configured manually and properly by the user, including the include directory setup.
Does that solve your problem here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! The last commit I pushed (2833fae) implements something like this. cythonize does not require the "np_pythran" flag anymore, and it is gathered from the cython header directives. The documentation has been updated accordingly!

@aguinet
Copy link
Contributor Author

aguinet commented Feb 25, 2017

Hello @scoder,

Thanks a lot for you reviews! I'm going through them and will push a commit that takes them into account.

For now it does not support memory views, as we'd have to convert them to Pythran objects. I think we could work on this after this PR is cleaned and merged :)

Copy link
Contributor

@scoder scoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes look good to me, added two more comments.

return is_type(type_,pythran_supported) or is_pythran_expr(type_)

def is_pythran_expr(type_):
return hasattr(type_, "is_pythran_expr") and type_.is_pythran_expr
return type_.is_pythran_expr
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems rather redundant now :)

@@ -579,6 +580,12 @@ def make_fused_cpdef(self, orig_py_func, env, is_def):
decl_code = Code.PyxCodeWriter(context=context)
decl_code.put_chunk(
u"""
cdef union {{lecheck_cname}}:
unsigned int i
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problem is, C int isn't guaranteed to be 4 bytes. And then, stdint.h isn't available in some MSVC versions...
I think we should provide the endianess check globally for the complete Cython module, i.e. do it in C and move it to ModuleSetupCode.c. There is an example for defining uint16_t in TypeConversion.c, uint32_t could be defined similarly. (Although ISTM that we should use a more safely escaped name than uint16_t there as well...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 55bbef3

@@ -659,6 +672,22 @@ static int __Pyx_check_binary_version(void) {
return 0;
}

/////////////// IsLittleEndian.proto ///////////////

static int __Is_Little_Endian(void);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have a __Pyx_ prefix to prevent naming collisions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@scoder
Copy link
Contributor

scoder commented Apr 9, 2017

Looks really good to me, although it would be nice if you could resolve the recent merge conflicts with current master. (Sorry for those.)
@robertwb, what do you think? Any objections to merging this?

@scoder
Copy link
Contributor

scoder commented Apr 9, 2017

Hmm, interesting, didn't know the new "resolve conflicts" feature in github yet. That was easy :) Ready for merging from my side then.

@aguinet
Copy link
Contributor Author

aguinet commented Apr 10, 2017

I have made a rebase to merge all Pythran-related commits into one, and renamed __Is_Little_Endian!

@aguinet
Copy link
Contributor Author

aguinet commented Apr 10, 2017

@scoder before merging, we need to answer the question about how to tell Cython to use Pythran. As stated above:

I started to add a np_pythran compiler attribute . I "just" need to get this information before compilation to add the include directory to the Pytran module. If I understand correctly, it seems that, for now, "only" distuils directives are taken into account at the compilation stage. If I understand correctly, I need to add a "parsing" the same way it is done for distutils directives.

Do I miss something ?

c_file = base + '.cpp'
options = pythran_options
m.include_dirs.append(pythran_include_dir)
m.extra_compile_args.extend(('-std=c++11','-DENABLE_PYTHON_MODULE','-D__PYTHRAN__'))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we read this list from the current Pythran installation somehow? It might change, and it would be nice if Cython wouldn't block this change in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This information can be read through:

from pythran.config import cfg
 cfg.get('compiler', 'cflags')

(without the -D__PYTHRAN__)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in cad11dc !

m.undef_macros.extend(pythran_ext['undef_macros'])
m.library_dirs.extend(pythran_ext['library_dirs'])
m.libraries.extend(pythran_ext['libraries'])
# These options are not compatible with the way normal Cython plugins work
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aguinet there's no guarantee these two options will be inserted by pythran (the user can customize these settings) so you should wrap each remove in a try block

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay thanks a lot I'm fixing it!

@aguinet aguinet force-pushed the feature/pythran branch 3 times, most recently from 59ef3b9 to a3b32bd Compare June 17, 2017 12:15
When the user asked for it (thanks to the --np-pythran flag), use
Pythran's Numpy implementation as a backend for numpy operation. This
flag forces the C++ mode, as the Pythran implementation is written in
C++. Distutils integration through the 'np_pythran' flag of the
cythonize API is also provided.

This commit also adds a Pythran mode for the tests, that can enable the
pythran mode for the C++ tests, and allows the reuse of Cython tests to
test for the Pythran integration.
This makes Pythran numpy backend only activated when such directive is
used. Update the documentation accordingly.
@scoder
Copy link
Contributor

scoder commented Jun 27, 2017

Thank you for coding this up. I'm happy to merge it now, and would be even happier to see general support for memory views in the future. :)

@scoder scoder merged commit 6030d91 into cython:master Jun 27, 2017
@aguinet
Copy link
Contributor Author

aguinet commented Jun 27, 2017

Thanks a lot for merging this @scoder ! :)

@scoder
Copy link
Contributor

scoder commented Jul 24, 2017

I noticed one problem with this. Pythran includes non-existant external files when NDEBUG is not defined, e.g. here:
https://github.com/serge-sans-paille/pythran/blob/26ed2646168e2045768dced260bf52b41a57dea2/pythran/pythonic/types/slice.hpp#L8
However, neither does the current build define this macro, nor should it IMHO, because only regular non-debug CPython builds define it. Is this a bug in Pythran, or should we do something about it on our side?

@scoder
Copy link
Contributor

scoder commented Jul 24, 2017

I pushed a test here:
scoder@45d49ff
Trying to compile it, I get tons of errors, e.g.

numpy_pythran.cpp:3949:58: error: no matching function for call to ‘pythonic::types::numpy_expr<pythonic::operator_::functor::mul, pythonic::types::broadcast<double, long int>, pythonic::types::numpy_gexpr<pythonic::types::ndarray<double, 2ul>, pythonic::types::contiguous_slice, pythonic::types::contiguous_slice> >::numpy_expr(<brace-enclosed initializer list>)’
     new (&__pyx_t_12) decltype(__pyx_t_12){2 * __pyx_t_11};

/usr/include/c++/5/tuple:105:22: error: use of deleted function ‘pythonic::types::broadcast<T, B>::broadcast() [with T = double; B = long int]’
       : _M_head_impl() { }

@aguinet Could you help me fix it up?

@serge-sans-paille
Copy link
Contributor

I'll have a look at this!

@serge-sans-paille
Copy link
Contributor

Should be fixed in serge-sans-paille/pythran#701, I don't know if you can check if it's ok on your side?

@scoder
Copy link
Contributor

scoder commented Jul 24, 2017

The NDEBUG compilation is fixed by serge-sans-paille/pythran#701, thanks.

Do you also have an idea about the test code I wrote (or rather: copied and adapted from Pythran's own test suite)? Do you see anything wrong with that? Or is there a bug in the integration code? I'm using this with Cython master and Pythran 0.8.1 (plus your NDEBUG patch).

@aguinet
Copy link
Contributor Author

aguinet commented Jul 25, 2017

Hello @scoder ,

Sorry I was in holidays! Your test code looks fine to me, I'll have a look in the day to see why it isn't compiling!

@serge-sans-paille
Copy link
Contributor

@scoder just so you know, I've started to integrate cython integration (minimal) testing in pythran test base to avoid breaking compatibility:

serge-sans-paille/pythran#704

@scoder
Copy link
Contributor

scoder commented Aug 5, 2017

Thanks! It's always good to notice it early when things break, and tests on both sides will help with that.

Which also means that we still need good tests on our side. ;)

@serge-sans-paille
Copy link
Contributor

yep, I currently only test a one liner, that's just the first seed :-)

@scoder
Copy link
Contributor

scoder commented Aug 20, 2017

@aguinet I still haven't managed to get any example to compile at all. I do assume that it worked for you when you implemented it, so I wonder what you are doing differently.

@serge-sans-paille
Copy link
Contributor

@scoder just so you know, I've manager to run a minimal example in the pythran test suite https://github.com/serge-sans-paille/pythran/tree/master/pythran/tests/cython

@serge-sans-paille
Copy link
Contributor

@scoder: I managed to get your example working in a local branch, see https://github.com/serge-sans-paille/pythran/commits/feature/cython-support.

The only change I add to do is replacing the 2 by 2L in cython input. @aguinet I leave that point to you.

@aguinet
Copy link
Contributor Author

aguinet commented Sep 4, 2017

@scoder : can you try using @serge-sans-paille 's pythran branch ( https://github.com/serge-sans-paille/pythran/commits/feature/cython-support) and this cython branch : https://github.com/aguinet/cython/tree/fix/integer_literal ?

The example in pythran's repository works for me with these branches with clang 4.0 ! If it's okay for you we'll make the associated PR.

@scoder
Copy link
Contributor

scoder commented Sep 9, 2017

Test updated, works for me now, with a couple of more preceding changes in Cython.
scoder@bbe5614
Casting apparently did the trick, but it requires specialising the Pythran expressions in several more places. I probably didn't find all of them yet. More tests would be good.

@scoder
Copy link
Contributor

scoder commented Sep 9, 2017

BTW, type inference for these expressions would be nice. Currently, when you do something like

    result1 = array * 5
    result2 = result * 2

Then you lose the expression context and everything is back in Python from the assignment on. Instead, the variables result1 and result2 should be inferred as Pythran expression values.

@scoder
Copy link
Contributor

scoder commented Sep 10, 2017

The next thing that I tried to compile was this:

# cython: language=c++
# cython: np_pythran=True

import numpy as np
cimport numpy as cnp

def calculate_tax_numpy_segments(cnp.ndarray[double, ndim=1] d):
    tax_seg1 = d[(d > 256303)] * 0.45 - 16164.53
    tax_seg2 = d[(d > 54057) & (d <= 256303)] * 0.42 - 8475.44
    seg3 = d[(d > 13769) & (d <= 54057)] - 13769
    seg4 = d[(d > 8820) & (d <= 13769)] - 8820
    prog_seg3 = seg3 * 0.0000022376 + 0.2397
    prog_seg4 = seg4 * 0.0000100727 + 0.14
    return (
        np.sum(tax_seg1) +
        np.sum(tax_seg2) +
        np.sum(seg3 * prog_seg3 + 939.57) +
        np.sum(seg4 * prog_seg4)
    ) / np.sum(d)

This passes through Cython but then fails in g++ with this error on the first temp declaration for d[(d > 256303)]:

module.cpp:3350:193: error: no match for call to ‘(pythonic::types::ndarray<double, 1ul>) (pythonic::types::numpy_expr<pythonic::operator_::functor::gt, pythonic::types::ndarray<double, 1ul>, pythonic::types::broadcast<double, long int> >)’

followed by several others. Do boolean array masks work? Or is this something different?

@serge-sans-paille
Copy link
Contributor

The following compiles fine with pythran, so I think it's an issue with the pythran+cython coupling

import numpy as np
#pythran export tax(float[])
def tax(d):
    tax_seg1 = d[(d > 256303)] * 0.45 - 16164.53
    tax_seg2 = d[(d > 54057) & (d <= 256303)] * 0.42 - 8475.44
    seg3 = d[(d > 13769) & (d <= 54057)] - 13769
    seg4 = d[(d > 8820) & (d <= 13769)] - 8820
    prog_seg3 = seg3 * 0.0000022376 + 0.2397
    prog_seg4 = seg4 * 0.0000100727 + 0.14
    return (
        np.sum(tax_seg1) +
        np.sum(tax_seg2) +
        np.sum(seg3 * prog_seg3 + 939.57) +
        np.sum(seg4 * prog_seg4)
    ) / np.sum(d)

@aguinet any thought?

@serge-sans-paille
Copy link
Contributor

ok, after digging through the error message, it looks like the array is indexed throught the operator() instead of operator[] by the cython to pythran bridge, which leads to the error, I'll check this with @aguinet

@scoder
Copy link
Contributor

scoder commented Sep 10, 2017

With the change below, I get it to compile. But enabling np_pythran mode now generates an incorrect result for me...

diff --git a/Cython/Compiler/ExprNodes.py b/Cython/Compiler/ExprNodes.py
index 41abed3..0bee0f7 100644
--- a/Cython/Compiler/ExprNodes.py
+++ b/Cython/Compiler/ExprNodes.py
@@ -4226,7 +4226,7 @@ class BufferIndexNode(_IndexingBaseNode):
         if is_pythran_expr(self.base.type):
             res = self.result()
             code.putln("__Pyx_call_destructor(%s);" % res)
-            code.putln("new (&%s) decltype(%s){%s(%s)};" % (
+            code.putln("new (&%s) decltype(%s){%s[%s]};" % (
                 res,
                 res,
                 self.base.pythran_result(),
diff --git a/Cython/Compiler/Pythran.py b/Cython/Compiler/Pythran.py
index 047edf3..0073fd3 100644
--- a/Cython/Compiler/Pythran.py
+++ b/Cython/Compiler/Pythran.py
@@ -76,7 +76,7 @@ def pythran_indexing_type(type_, indices):
         raise ValueError("unsupported indexing type %s!" % idx.type)
 
     indexing = ",".join(index_code(idx) for idx in indices)
-    return type_remove_ref("decltype(std::declval<%s>()(%s))" % (pythran_type(type_), indexing))
+    return type_remove_ref("decltype(std::declval<%s>()[%s])" % (pythran_type(type_), indexing))
 
 
 def pythran_indexing_code(indices):

@serge-sans-paille
Copy link
Contributor

I cna reproduce the issue, keep in touch :-)

@serge-sans-paille
Copy link
Contributor

@scoder should be fixed in pythran branch feature/cython-support

@scoder
Copy link
Contributor

scoder commented Sep 11, 2017

Fix confirmed. I'll also apply my patch above to Cython then.

@scoder
Copy link
Contributor

scoder commented Sep 11, 2017

Note that I'm now seeing this error for the previous test I wrote in
scoder@bbe5614

In file included from .../pythran/pythonic/types/ndarray.hpp:41:0,
                 from numpy_pythran.cpp:516:
.../pythran/pythonic/types/numpy_gexpr.hpp: In instantiation of ‘pythonic::types::numpy_gexpr<A, S>::numpy_gexpr() [with Arg = const pythonic::types::ndarray<double, 2ul>&; S = {pythonic::types::contiguous_slice}]’:
numpy_pythran.cpp:3940:181:   required from here
.../pythran/pythonic/types/numpy_gexpr.hpp:342:5: error: uninitialized reference member in ‘const struct pythonic::types::ndarray<double, 2ul>&’ [-fpermissive]
     numpy_gexpr<Arg, S...>::numpy_gexpr()
     ^
In file included from .../pythran/pythonic/include/types/ndarray.hpp:39:0,
                 from .../pythran/pythonic/types/ndarray.hpp:4,
                 from numpy_pythran.cpp:516:
.../pythran/pythonic/include/types/numpy_gexpr.hpp:384:11: note: ‘const pythonic::types::ndarray<double, 2ul>& pythonic::types::numpy_gexpr<const pythonic::types::ndarray<double, 2ul>&, pythonic::types::contiguous_slice>::arg’ should be initialized
       Arg arg;

@serge-sans-paille
Copy link
Contributor

@scoder I just pushed a quick'n dirty hack around that issue in feature/cython-support, can you confirm everything is green on your side, I'm planning to do a release soon.

@serge-sans-paille
Copy link
Contributor

@scoder all the commits related to better cython interaction have been merged in the latest release (0.8.2, on pip and conda). Hope it helps.

@serge-sans-paille
Copy link
Contributor

@scoder, @aguinet : serge-sans-paille/pythran#725 fixes the issue mentionned in last @aguinet email.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants