xtensor-stack / xtensor

C++ tensors with broadcasting and lazy computing
BSD 3-Clause "New" or "Revised" License
3.33k stars 398 forks source link

signed/unsigned compile issues with xtensor 0.11.3 #594

Closed blackccpie closed 6 years ago

blackccpie commented 6 years ago

Hi,

I'm working on upgrading currently embedded tiny-dnn's xtensor version (0.10.9) to version 0.11.3, but I have some compilation issues. I've isolated my problem in the following code:

#include "xtensor/xarray.hpp"
#include "xtensor/xview.hpp"

template <typename U = float_t, typename Storage = xt::xarray<U>>
class Tensor {

 public:

  /**
   * Constructor that accepts an initializer list of shape and create a
   * Tensor with that shape and filling it with value. For example,
   * given shape = {2,3,4,5,6}, value = 0 tensor will be of size 2x3x4x5x6
   * filled with zeros
   * @param shape  shape array containing N integers, sizes of dimensions
   * @param value value to fill
   */
  explicit Tensor(std::initializer_list<size_t> const &shape, U value)
    : storage_(shape, value) {}

  /**
   * Returns view of current Tensor
   * @tparam Values index type
   */
   template <typename... Values>
   Tensor<U, xt::xview<Storage &, xt::xrange<Values>...>> subView(
     std::initializer_list<Values>... lists) {
     using SharedTensor = Tensor<U, xt::xview<Storage &, xt::xrange<Values>...>>;
     return SharedTensor(storage_,
                         xt::range(*(lists.begin()), *(lists.begin() + 1))...);
   }

  /**
   * Creates Tensor given the storage
   * @tparam T
   * @param storage
   */
  template <class T, class S, class... Args>
  explicit Tensor(T &storage, xt::xrange<S> r1, Args... args)
    : storage_(xt::view(storage, r1, args...)) {}

 private:
  Storage storage_;
};

int main()
{
    Tensor<float_t> tensor({3, 3, 3, 3}, 2);
    auto t_view = tensor.subView({0, 2}, {0, 1}, {0, 3}, {0, 3});

    return 0;
}

If I replace the subView call in the main function by this one then it compiles fine:

auto t_view = tensor.subView({0u, 2u}, {0u, 1u}, {0u, 3u}, {0u, 3u});

So the problem is related to signed/unsigned conversion issues, but I don't know what's the cleanest way to correct this in the subView method. I've been struggling to try to use std::make_unsigned_t to operate on the parameter pack, without success for now...

Could someone help me on this? Thx

Albert

blackccpie commented 6 years ago

Finally I made this work with std::make_unsigned_t:

 template <typename... Values>
   Tensor<U, xt::xview<Storage &, xt::xrange<std::make_unsigned_t<Values>>...>> subView(
     std::initializer_list<Values>... lists) {
     using SharedTensor = Tensor<U, xt::xview<Storage &, xt::xrange<std::make_unsigned_t<Values>>...>>;
     return SharedTensor(storage_,
                         xt::range(*(lists.begin()), *(lists.begin() + 1))...);
   }

Anyway is it really normal to have this constraint using xrange?

JohanMabille commented 6 years ago

Hi, sorry for the late reply.

Unfortunately this constraint is normal; xrange can be instantiated with signed integers to avoid mixing signed / unsigned arithmetic when used in a view also containing an xstepped_range with a negative step for instance.

JohanMabille commented 6 years ago

Note that with xtensor 0.14.0, the xrange takes three template parameters, and your first code:

int main()
{
    Tensor<float_t> tensor({3, 3, 3, 3}, 2);
    auto t_view = tensor.subView({0, 2}, {0, 1}, {0, 3}, {0, 3});

    return 0;
}

should compile fine without using any std::make_unsigned_t. The range will be instantiated with signed integers, though.

blackccpie commented 6 years ago

@JohanMabille : thx for the precisions. For now I targeted an xtensor version not depending on xtl, but I will try to remember your hint for a future update.

JohanMabille commented 6 years ago

@blackccpie no problem. Out of curiosity, what does prevent you from depending on xtl?

blackccpie commented 6 years ago

@JohanMabille : the thing is I just needed tiny-dnn to compile fine with emscripten, and as xtensor is embedded in tiny-dnn, and as I'm not a main contributor of the project, I wanted my PR to have limited impact on third party dependencies.

blackccpie commented 6 years ago

@JohanMabille : but all things considered I admit that shouldn't be too much work to embed xtl headers alongside with xtensor.

JohanMabille commented 6 years ago

ah indeed, I forgot that tiny-dnn was embedding xtensor headers (rather than only depend on them). Indeed embedding xtl headers should be quite straightforward, but I agree that many PRs with limited impact are better than a single one with big changes.