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

adding xt::detail::has_fixed_size and replacing is_array to expand supported types. #2556

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vakokako
Copy link

@vakokako vakokako commented Jul 18, 2022

This pr adds type trait xt::detail::has_fixed_size to replace a common usage of xt::detail::is_array where implementation can benefit if the size of the container (usually used for shape) is known at compile time. This allows to add xt::sequence_view for example to supported types for shape in xt::adapt, but even much much more.

This also adds xt::detail::get_fixed_shape as a wrapper to std::tuple_size. The reason this is needed is that for example for xfunction that has fixed shape, we also want to use std::tuple_size, but we cannot partially specialize std::tuple_size only for the cases when xfunction has fixed shape.

auto tensor = xt::eval(xt::zeros<double>(xt::adapt(std::array<size_t, 2>{}) / 2));
// before decltype(tensor) == xarray
// after decltype(tensor) == xtensor

@vakokako vakokako force-pushed the replacing_is_array_with_has_fixed_size_in_xadapt branch 2 times, most recently from 1dd2a56 to 037ef36 Compare July 26, 2022 15:38
@vakokako
Copy link
Author

The build only fails on the older msvc 19.0, but it's not clear why. It says use of undefined type 'xt::detail::get_fixed_size<T,void>', but this should use the specialization in xshape.hpp at line 399.

Copy link
Member

@JohanMabille JohanMabille left a comment

Choose a reason for hiding this comment

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

The error on Windows seems legit.

# pragma clang diagnostic ignored "-Wmismatched-tags"
#endif

namespace std
Copy link
Member

Choose a reason for hiding this comment

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

A good practice is to avoid opening std namespace. You can directly specialize tuple_size with the following syntax:

tempate <class ET, class S, xt::layout_type L, bool SH, class TAG>
struct std::tuple_size<xt::xfixed_container<ET, S, L, SH, Tag>> : 
// ...

Also pay attention that tuple_size is a struct, not a class (therefore, no need to specify a public inheritance, and this will avoid a lot of warnings).

Copy link
Author

Choose a reason for hiding this comment

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

This specialization was done in the same style as here:

class tuple_size<xt::const_array<T, N>> :

It also contains a comment about tuple_size being a class in c++ standard, I can just copy the comment here, or update also the xstorage version.

Copy link
Member

Choose a reason for hiding this comment

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

My initial comment was based on the standard definition of tuple_size. However, it appears that th standard uses class tuple_size in other parts of the spec, and is therefore inconsistent. libc++ is consistent and uses class everywhere, so we can keep it. See this discussion for more detail about this issue.

The specializations in xstorage.hpp should be updated to avoid opening the std namespace, though.

include/xtensor/xfunction.hpp Outdated Show resolved Hide resolved
include/xtensor/xfunction.hpp Outdated Show resolved Hide resolved
@vakokako vakokako force-pushed the replacing_is_array_with_has_fixed_size_in_xadapt branch from 037ef36 to 351378a Compare July 27, 2022 08:54
@vakokako vakokako force-pushed the replacing_is_array_with_has_fixed_size_in_xadapt branch from 956e95f to 22a30c9 Compare August 8, 2022 13:49
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.

2 participants