xindexview function for advanced indexing

[wolfv]

I hope this doesn't come across as spam :) but I have attempted an implementation for an "index view". ie. one can provide a list of indices and those will be selected into a linear view.
Essentially, it allows for numpys advanced indexing like so:

	xt::xarray<double> rn = xt::random::rand<double>({3, 3}, 0, 1);

	auto v = xt::make_xindexview(rn, {{1,1}, {2,2}});
	auto v2 = xt::make_boolview(rn, (rn > 0.2));
	std::cout << v << std::endl;
	std::cout << v2 << std::endl;
	v += 2.0;

	v2 += 15;
	std::cout << rn << std::endl;

With corresponding output:

{{0.135477, 0.835009, 0.968868},
 {0.221034, 0.308167, 0.547221},
 {0.188382, 0.992881, 0.996461}}

{0.308167, 0.996461}

{0.835009, 0.968868, 0.221034, 0.308167, 0.547221, 0.992881, 0.996461}

{{0.135477, 15.835, 15.9689},
 {15.221, 17.3082, 15.5472},
 {0.188382, 15.9929, 17.9965}}

Let me know what you think.

[JohanMabille]

Who will stop you ? :)

That's definitely a must have. I guess this implementation is not finished (since some operators are commented and there is no test) ?

[wolfv]

So this was functionality wise already working.

However, I am wondering: Does it make sense to implement xstepper in a way that it takes a reference to the underlying m_indices and uses the begin() end() of the underlying vector of indices?
If you think yes, then I'd try to improve the xstepper in that regard.

I'll also add some tests!

[JohanMabille]

If that improves the performance of the stepper, that makes sense. Else you should avoid to use the private implementation of the indexview and rely on its interface.

[wolfv]

Ok, I have implemented the xstepper with the public interface of the underlying view.

Let me know what you think!

Currently can't really figure out why the clang 3.6 build is failing.

I think one improvement could be to use a heap allocated shape if a compile time known list of indices is given.

[JohanMabille]

That looks like a dangling reference bug. We experienced it before on other tests, a workaround is to avoid nested calls to functions that return xview and/or xfunction. #85 should fix this kind of problems.

I'll have a look at your code soon !

[wolfv]

I'm giving up ... is there an easy way to install old clang in a docker image?

[JohanMabille]

I don't know how to do that :/ Maybe you could try running the tests with valgrind ? Even if the tests pass, valgrind could detect some memory issues that lead to the crash with clang.

[wolfv]

Ok, I installed clang3.6 in a docker image and found the bug ...

when return x[i]++ clang3.6 apparently returns the element of the array (x[i]) instead of x ... :)

[JohanMabille]

Well done !

[JohanMabille]

This should be replaced by the documentation corresponding to this class. The same for the remaining of the documentation

[JohanMabille]

It's only about style here, but I think T should be replaced by E. T generally implies a basic type parameter, while E implies more an expression, which is the kind of types you expect here.

[JohanMabille]

Iterators should be passed by value, not by const ref.

[JohanMabille]

Same remark as for the non const reference

[JohanMabille]

I think there's a typo here, you can't combine const and rvalue reference. Depending on how you want to instantiate an xindexvie you might consider rvalue reference and const lvalue reference overloads, or better universal references.

[JohanMabille]

Same remark as for the class declaration, you should rewrite the documentation so it matches what this code does.

[JohanMabille]

Same remark as for the xindex_view constructor, you can't mix const and rvalue reference. You can't use universal references if you want to allow initializer lists, so you'll need overloads for rvalue reference and const lvalue reference.

[wolfv]

Hmm... I have fixed this issue. Can you check if I got it right? I'm not a super-expert on these fancy references :)

[JohanMabille]

The constructor of xindex_view is good.

The make_xindexview function may be problematic, generally you should avoid to overload on universal references since they are a "catch all" arguments. However this seems to work and I don't want to wait more for this feature (Travis delays are killing me).

[JohanMabille]

I wonder whether something like filter_view or make_xfilter would be a better name.

[wolfv]

renamed it to make_xfilter for now.

[JohanMabille]

I'm not sure about it but shouldn't we have m_index[dim] -= n ?

[wolfv]

fixed.

[JohanMabille]

I add it to 0.4 milestone since you're very close to finish this.
Should fix #105.

[JohanMabille]

Let's merge !