xtensor-stack / Xtensor.jl

Julia package for xtensor-julia
http://quantstack.net/xtensor
BSD 3-Clause "New" or "Revised" License
41 stars 6 forks source link

Add more julia tests #40

Closed SylvainCorlay closed 7 years ago

SylvainCorlay commented 7 years ago

This PR reproduces the tests that we have in xtensor-python. Current issues.

1) CxxWrap's add_lambda function takes its third argument (the pointer to operator()) const, which prevents the use of functors where operator() is not const qualified.

_Question_: is there a deep reason for the third argument of `add_lambda` to be `const`? cc @barche 

2) Unlike numpy arrays, in Julia, N-D arrays with different numbers of dimensions have different types. So in order to return an array with a number of dimension determined at runtime (such as for numpy-style broadcasting), we must create the type at runtime, which is what we do in the specialization of ConvertToJulia::operator().

Now, when using `module.method` with a function, this uses `mapped_return_type` which relies on the typedefs of `mapped_return_type`. By chance, regardless of the dimension, this is always `jl_array_t*`.

For this reason, even though we use dynamic type mapping, we must override static_type_mapping for the typedefs here.

barche commented 7 years ago

Nope, it not accepting non-const operators was a bug :)

SylvainCorlay commented 7 years ago

Nope, it not accepting non-const operators was a bug :)

Thanks. PS: Actually my PR did not address that but only the forwarding issue. In the case LambdaT is const, operator() is still of const type. Looking at a better way to infer the signature now.

SylvainCorlay commented 7 years ago

@barche would it make sense to add a static assert that attribute types of functions passed to method are not reference types?

barche commented 7 years ago

What do you mean by attribute types?

SylvainCorlay commented 7 years ago

I mean that binding foo(ArrayRef<double>& x) does not work but foo(ArrayRef<double> x) does. The former yields a dangling reference.

barche commented 7 years ago

Did you try that? I get a compile error when I use a non-const ref:

error: non-const lvalue reference to type 'cxx_wrap::ArrayRef<double, 1>' cannot bind to a temporary of type 'jl_array_t *'

With a const ref it does work, the temporary returned from the conversion survives until the function is done.

SylvainCorlay commented 7 years ago

Although if you want to use the ArrayRef to modify the underlying julia array, it should be non const, so passing it by value is recommended right?

SylvainCorlay commented 7 years ago

Hum, un-commenting example1 triggers a segfault in the Julia testing. This is surprising because we are actually not even calling the function.

barche commented 7 years ago

If I'm reading the code correctly, you're returning a nullptr in static_type_mapping::julia_type, that would be a likely cause for the segfault.

Regarding the const ArrayRef: yes, to call a non-const method on that passing by value is better, even though you could always copy the const ref and it would still modify the same array.

SylvainCorlay commented 7 years ago

So the issue is that the type mapping is not static since I compute the Julia type dynamically based on the dimension of the jlarray. The specialization of ConvertToJulia does not make use of julia_type.

barche commented 7 years ago

The static part of the mapping is used to determine the signature of the function passed to ccall, but julia_type is used at runtime when the Julia wrapper methods are generated. Having this return a nullptr will attempt to create a Julia method with a return or argument type that is a nullptr, so this will crash when the module is loaded by wrap_module.

SylvainCorlay commented 7 years ago

What is the consequence of apply_array_type being called with the wrong number of dimensions in this context?

barche commented 7 years ago

I would just omit the type parameters then, returning the base type using e.g.

return cxx_wrap::julia_type("AbstractArray");
SylvainCorlay commented 7 years ago

@barche FYI,

static jl_datatype_t* julia_type()
{
    // Array{T}
    return (jl_datatype_t*)apply_type(
        jl_get_global(jl_current_module, jl_symbol("Array")),
        jl_svec1(cxx_wrap::julia_type<T>())
    );
}
SylvainCorlay commented 7 years ago

Also, in the CI, I noticed that Julia's Int is either int64_t or in32_t depending on the platform, which is ptrdiff_t. I was thinking that having a jl_int_t typedef might be nice.

barche commented 7 years ago

Julia already defines int_t and uint_t for these, but they are well hidden in dtypes.h, which is included from julia.h.

SylvainCorlay commented 7 years ago

Cool! Thanks for the info Do you plan on releasing CxxWtap 0.4 soonish? Or should I not wait for it before tagging a first release of xtensor-julia?

barche commented 7 years ago

As far as I can tell the basics needed to split off the C++ part of CxxWrap into a separate package is now complete, so I think I can tag 0.4, yes. One thing I would still like to check before is how VS2017 and VS2015 can be supported at the same time.

SylvainCorlay commented 7 years ago

Eventually, it would be nice to have a clear story of how we handle not vendoring binary artefacts and headers in the the julia package directory, for contexts such as Anaconda or linux distributions.

For examples, having a flag for build.jl to pass another installation prefix for cmake.