Description

ThibHlln

opened on Jan 31, 2023

Issue body actions

Hi,

The new added xt::quantile seems not to play nicely with pytensor.

For me, the following :

xt::pytensor<double, 2> arr
            {{ 5.3, 4.3, 5.3 },
             { 4.2, 4.2, 5.2 },
             { 5.7, 4.7, 5.7 },
             { 2.3, 4.3, 2.3 }};

auto q = xt::quantile(arr, {0.3, 0.7}, 1);

raises :

~/xtensor/include/xtensor/xsort.hpp:463:31: error: non-constant-expression cannot be narrowed from type 'xt::xcontainer<xt::pytensor<double, 2, xt::layout_type::dynamic>>::size_type' (aka 'unsigned long') to 'long' in initializer list [-Wc++11-narrowing]
R ev = R::from_shape({de.size()});

I am using xtl==0.7.5, xtensor==0.24.4, xtensor-python==0.26.1, pybind11==2.10.3.

Activity

1. JohanMabille commented on Jan 31, 2023

   

   JohanMabille

   on Jan 31, 2023

   Member

   More actions

   Thanks for reporting. This is definitely a bug in xtensor.

   Reacted by Thibault Hallouin
2. ThibHlln commented on Feb 17, 2023

   

   ThibHlln

   on Feb 17, 2023

   Author

   More actions

   Hi @AntoinePrv, do you think that this issue is related to xtensor-stack/xtensor#2651, and so would be fixed by xtensor-stack/xtensor#2652 too, or is this an unrelated problem?
3. AntoinePrv commented on Feb 20, 2023

   

   AntoinePrv

   on Feb 20, 2023

   More actions

   @ThibHlln I could not reproduce this. Is this a compiler error? If so which one and on which platform?

   I does not seem related to xtensor-stack/xtensor#2651 but perhaps since xtensor-stack/xtensor#2651 is a big refactor it could solve it. Do you have the time to try?
4. ThibHlln commented on Feb 21, 2023

   

   ThibHlln

   on Feb 21, 2023

   Author

   More actions

   Hi @AntoinePrv,

   Yes, this is a compiler error I get with Clang on macOS, and also with MSVC on Windows (see error message below).

   C:\Users\thallouin\micromamba\envs\pytensor-sandbox-env\Library\include\xtensor/xsort.hpp(463,38): error C2397: conversion from 'unsigned __int64' to '_Ty' requires a narrowing conversion [C:\Users\thallouin\CLionProjects\pytensor-sandbox\build\pytensor_sandbox.vcx
   proj]
   with
   [
   _Ty=ptrdiff_t
   ]
   C:\Users\thallouin\micromamba\envs\pytensor-sandbox-env\Library\include\xtensor/xsort.hpp(508): message : see reference to function template instantiation 'R xt::partition<E,C,eval_type,int>(const xt::xexpression &,const C &,xt::placeholders::xtuph)' being compi
   led [C:\Users\thallouin\CLionProjects\pytensor-sandbox\build\pytensor_sandbox.vcxproj]
   with
   [
   R=eval_type,
   E=xt::pytensor<double,2,xt::layout_type::dynamic>,
   C=xt::xtensor_container<xt::uvector<size_t,std::allocator<size_t>>,1,xt::layout_type::row_major,xt::xtensor_expression_tag>,
   D=xt::pytensor<double,2,xt::layout_type::dynamic>
   ]

   So, I tried with your PR xtensor-stack/xtensor#2652, and even though it does not look like you touched the line the compiler is complaining about (i.e. https://github.com/xtensor-stack/xtensor/blob/master/include/xtensor/xsort.hpp#L463), I don't get the error anymore on Windows with MSVC (I will check on macOS with Clang later on), so maybe the type of the variable has changed and the narrowing conversion is not necessary anymore?
5. AntoinePrv commented on Feb 22, 2023

   

   AntoinePrv

   on Feb 22, 2023

   More actions

   Basically in xt::partition (and other places), a tensor gets created with R::from_shape({ de.size() });
   https://github.com/xtensor-stack/xtensor/blob/master/include/xtensor/xsort.hpp#L463
   For xt::pytensor size_type is unsigned (derived from buffer_adaptor), but shape_type (the argument of from_shape) is std::array<npy_intp, N> hence the narrowing conversion.
   A fix there would be simple enough but let's see if it is still present with xtensor-stack/xtensor#2652.

   IMHO the real issue here, and in other bugs we've caught, is that xtensor functions are not tested consistently with all types of containers (let alone view, expressions). Tracking in xtensor-stack/xtensor#2658.
6. ThibHlln commented on Feb 22, 2023

   

   ThibHlln

   on Feb 22, 2023

   Author

   More actions

   OK, thank you for the explanation @AntoinePrv.

   Even though my MWE provided above only failed with MSVC on Windows, in a more complex project, I had the same kind of failure with Clang on macOS. But once again, using xtensor-stack/xtensor#2652 fixed it, so I am looking forward to the PR merge now. :-)
7. stellarpower commented on Mar 2, 2023

   

   stellarpower

   on Mar 2, 2023

   More actions

   Just FYI, I hit the exact same error message when I didn't have the Python headers included as system headers (i.e. -I vs -isystem). I think something in there is redefining or otherwise interfering with the size of builtin types, which would make sense re interfacing with Numpy. So this could be a build setup thing, I just fixed it in CMake myself.

   Reacted by Thibault Hallouin

Issue actions