kokkos / mdspan

Reference implementation of mdspan targeting C++23
Other
404 stars 69 forks source link

Should mdarray inherit mdspan? Fold mdarray into mdspan? #134

Closed Steve132 closed 1 year ago

Steve132 commented 2 years ago

It seems like it's very very important for the array specification that mdarray<Args...> should inherit from mdarray<Args...>::view_type. or perhaps there should be a series of implicit conversation operators defined to allow that implicit conversion.

When actually writing libraries that use mdarray as a base, then the natural desire is to write code such as

typename<class T,class ...Args>
T mathlib::sum(const std::mdspan<float,Args...>& x);

typename<class T>
std::mdarray<T,3,1> mathlib::cross(const std::mdspan<float,3,1>& x,const std::mdspan<float,3,1>& y);

Which results in user code such as

std::mdarray<float,3,1> a{a1,a2,a3},b{b1,b2,b3};
float result=sum(cross(a.view(),b.view()).view());

Of course, such code is really really verbose and difficult to read versus the natural usage:

    float result=sum(cross(a,b));

The thing is, this example is minimal and still provides 3 superfluous uses of .view().

As a library developer, the simplest and easiest solution to fix this usability defect is to switch to an owning mdarray globally for all interfaces to all algorithms, which of course means performance and compatibility penalties and memory copies when trying to pass an existing non-owning mdspan as an argument to an algorithm in the library. And that has no real benefit over existing custom library types (which is the whole point of what mdspan is trying to accomplish).

So, I strongly propose that the mdarray proposal be amended while it still can in such a way so that it implicitly can degrade to a span so that it can be used anywhere a span can be used, in order to maximize the liklihood that library developers migrate to mdspan arguments instead of restricting the library interface to only accept owned data.

An alternative that seems like it might be functional would be to actually entirely merge the mdspan and mdarray proposals into only the semantics of the mdarray proposal. This is because an mdspan is essentially just an mdarray with the containerpolicy set to be a std::span. So rather than having two interfaces in the api which are mutually incompatible, library developers could target one api. In the vast majority of cases, ownership or non-ownership isn't immediately important or relevant to api interface design, so targeting one api in that case makes usage and api design signiticantly easier...and in the cases where it is relevant, the containerpolicy can be inspected at compile time using concepts. This seems like it would be much much better than having two completely separate mutually exclusive apis which do not interoperate..

mhoemmen commented 2 years ago

Thank you for the suggestions!

I can't speak for the other coauthors, but I would prefer implicit conversion over inheritance. Precedence in the Standard Library is that string converts to string_view via string's operator basic_string_view. A container is not a view, so making mdarray inherit from mdspan would violate substitutability.

crtrott commented 2 years ago

Yeah definitely conversion. In fact the proposal has a conversion operator. However that will not trigger for

template<class T, lass E, class L, class A>
void foo(mdspan<T,E,L,A> a) {}

since it can't deduce the template arguments.

But I think we can fix that by proposing a deduction guide for mdspan, from mdarray, and a constructor for mdspan from mdarray.

Steve132 commented 2 years ago

But I think we can fix that by proposing a deduction guide for mdspan, from mdarray, and a constructor for mdspan from mdarray.

This is fully acceptable to me