Skip to content
This repository has been archived by the owner on Mar 21, 2024. It is now read-only.

transform_inclusive_scan broken with different value types #1260

Closed
rongou opened this issue Aug 25, 2020 · 3 comments · Fixed by #1261
Closed

transform_inclusive_scan broken with different value types #1260

rongou opened this issue Aug 25, 2020 · 3 comments · Fixed by #1261
Milestone

Comments

@rongou
Copy link
Collaborator

rongou commented Aug 25, 2020

With previous versions of Thrust, we used to be able to pass in iterators of different value types to thrust::transform_inclusive_scan for the transform the inclusive scan stages, with the UnaryFunction converting between the two. For example, https://github.com/rapidsai/cudf/blob/branch-0.16/cpp/src/copying/concatenate.cu#L74. But this seems to be broken with the version at head:

/home/rou/src/cudf/cpp/build/_deps/thrust-src/thrust/iterator/transform_iterator.h(316): error: no suitable constructor exists to convert from "size_t" to "cudf::column_device_view"
          detected during:
            instantiation of "thrust::detail::transform_iterator_base<AdaptableUnaryFunction, Iterator, Reference, Value>::type::reference thrust::transform_iterator<AdaptableUnaryFunction, Iterator, Reference, Value>::dereference() const [with AdaptableUnaryFunction=lambda [](const auto &)->auto, Iterator=thrust::detail::normal_iterator<const cudf::column_device_view *>, Reference=cudf::column_device_view, Value=thrust::use_default]" 
/home/rou/src/cudf/cpp/build/_deps/thrust-src/thrust/iterator/iterator_facade.h(128): here
            instantiation of "Facade::reference thrust::iterator_core_access::dereference(const Facade &) [with Facade=thrust::transform_iterator<lambda [](const auto &)->auto, thrust::detail::normal_iterator<const cudf::column_device_view *>, cudf::column_device_view, thrust::use_default>]" 
/home/rou/src/cudf/cpp/build/_deps/thrust-src/thrust/iterator/iterator_facade.h(310): here
            instantiation of "thrust::iterator_facade<Derived, Value, System, Traversal, Reference, Difference>::reference thrust::iterator_facade<Derived, Value, System, Traversal, Reference, Difference>::operator*() const [with Derived=thrust::transform_iterator<lambda [](const auto &)->auto, thrust::detail::normal_iterator<const cudf::column_device_view *>, cudf::column_device_view, thrust::use_default>, Value=cudf::column_device_view, System=thrust::host_system_tag, Traversal=std::random_access_iterator_tag, Reference=cudf::column_device_view, Difference=std::ptrdiff_t]" 
/home/rou/src/cudf/cpp/build/_deps/thrust-src/thrust/system/detail/sequential/scan.h(67): here
            instantiation of "OutputIterator thrust::system::detail::sequential::inclusive_scan(thrust::system::detail::sequential::execution_policy<DerivedPolicy> &, InputIterator, InputIterator, OutputIterator, BinaryFunction) [with DerivedPolicy=thrust::system::cpp::detail::par_t, InputIterator=thrust::transform_iterator<lambda [](const auto &)->auto, thrust::detail::normal_iterator<const cudf::column_device_view *>, cudf::column_device_view, thrust::use_default>, OutputIterator=thrust::detail::normal_iterator<size_t *>, BinaryFunction=thrust::plus<size_t>]" 
/home/rou/src/cudf/cpp/build/_deps/thrust-src/thrust/detail/scan.inl(63): here
            instantiation of "OutputIterator thrust::inclusive_scan(const thrust::detail::execution_policy_base<DerivedPolicy> &, InputIterator, InputIterator, OutputIterator, AssociativeOperator) [with DerivedPolicy=thrust::system::cpp::detail::par_t, InputIterator=thrust::transform_iterator<lambda [](const auto &)->auto, thrust::detail::normal_iterator<const cudf::column_device_view *>, cudf::column_device_view, thrust::use_default>, OutputIterator=thrust::detail::normal_iterator<size_t *>, AssociativeOperator=thrust::plus<size_t>]" 
/home/rou/src/cudf/cpp/build/_deps/thrust-src/thrust/system/detail/generic/transform_scan.inl(57): here
            instantiation of "OutputIterator thrust::system::detail::generic::transform_inclusive_scan(thrust::execution_policy<ExecutionPolicy> &, InputIterator, InputIterator, OutputIterator, UnaryFunction, BinaryFunction) [with ExecutionPolicy=thrust::system::cpp::detail::par_t, InputIterator=thrust::detail::normal_iterator<const cudf::column_device_view *>, OutputIterator=thrust::detail::normal_iterator<size_t *>, UnaryFunction=lambda [](const auto &)->auto, BinaryFunction=thrust::plus<size_t>]" 
/home/rou/src/cudf/cpp/build/_deps/thrust-src/thrust/detail/transform_scan.inl(47): here
            instantiation of "OutputIterator thrust::transform_inclusive_scan(const thrust::detail::execution_policy_base<DerivedPolicy> &, InputIterator, InputIterator, OutputIterator, UnaryFunction, AssociativeOperator) [with DerivedPolicy=thrust::system::cpp::detail::par_t, InputIterator=thrust::detail::normal_iterator<const cudf::column_device_view *>, OutputIterator=thrust::detail::normal_iterator<size_t *>, UnaryFunction=lambda [](const auto &)->auto, AssociativeOperator=thrust::plus<size_t>]" 
/home/rou/src/cudf/cpp/src/copying/concatenate.cu(74): here

Probably shouldn't assume the InputIterator and OutputIterator would have the same value type.

@dkolsen-pgi
Copy link
Collaborator

This is a bug in the C++ standard (which I think is now an LWG issue), which is repeated in P0571. For the transform_inclusive_scan overload that has no initial value, the intermediate type should be the result type of the transformation operation, not the value type of the input iterator.

@rongou
Copy link
Collaborator Author

rongou commented Aug 25, 2020

Can we fix it in Thrust or do we have to stick with the standard?

@dkolsen-pgi
Copy link
Collaborator

It should definitely be fixed in Thrust. It is an obvious bug with an obvious fix.

@brycelelbach brycelelbach added this to the 1.10.0 milestone Sep 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants