Add transform_input_output_iterator

[trevorsm7]

Adds a variant of transform iterator adapter that works as both an input iterator and an output iterator. The given input function is applied after reading from the wrapped iterator while the output function is applied before writing to the wrapped iterator. The implementation is largely based on transform_output_iterator, with additional operators added to the proxy reference.

Also fixes some typos in transform_output_iterator.

[alliepiper]

I haven't gone through the patch yet, but I did hit a build error when using non-CUDA backends. Looks like a missing header in the example:

/home/av/code/utils/ccache-install/bin/g++-7  -DTHRUST_DEVICE_SYSTEM=THRUST_DEVICE_SYSTEM_CPP -DTHRUST_HOST_SYSTEM=THRUST_HOST_SYSTEM_CPP -DTHRUST_IGNORE_DEPRECATED_CPP_11 -I/home/av/code/src/thrust/examples -I/home/av/code/src/thrust -O3 -DNDEBUG   -Wno-unused-parameter -Werror -Wall -Wextra -Winit-self -Woverloaded-virtual -Wcast-qual -Wno-cast-align -Wno-long-long -Wno-variadic-macros -Wno-unused-function -Wno-unused-variable -Wlogical-op -Wno-noexcept-type -std=c++14 -MD -MT examples/CMakeFiles/thrust.cpp.cpp.cpp14.example.transform_io_iterator.dir/thrust.cpp.cpp.cpp14/transform_io_iterator.cu.cpp.o -MF examples/CMakeFiles/thrust.cpp.cpp.cpp14.example.transform_io_iterator.dir/thrust.cpp.cpp.cpp14/transform_io_iterator.cu.cpp.o.d -o examples/CMakeFiles/thrust.cpp.cpp.cpp14.example.transform_io_iterator.dir/thrust.cpp.cpp.cpp14/transform_io_iterator.cu.cpp.o -c examples/thrust.cpp.cpp.cpp14/transform_io_iterator.cu.cpp
In file included from examples/thrust.cpp.cpp.cpp14/transform_io_iterator.cu.cpp:1:0:
/home/av/code/src/thrust/examples/transform_io_iterator.cu: In function ‘int main()’:
/home/av/code/src/thrust/examples/transform_io_iterator.cu:69:11: error: ‘sequence’ is not a member of ‘thrust’
   thrust::sequence(A.begin(), A.end(), 1);
           ^~~~~~~~
/home/av/code/src/thrust/examples/transform_io_iterator.cu:69:11: note: suggested alternative: ‘reduce’
   thrust::sequence(A.begin(), A.end(), 1);
           ^~~~~~~~
           reduce
/home/av/code/src/thrust/examples/transform_io_iterator.cu:70:11: error: ‘sequence’ is not a member of ‘thrust’
   thrust::sequence(B.begin(), B.end(), 5);
           ^~~~~~~~
/home/av/code/src/thrust/examples/transform_io_iterator.cu:70:11: note: suggested alternative: ‘reduce’
   thrust::sequence(B.begin(), B.end(), 5);
           ^~~~~~~~
           reduce
ninja: build stopped: subcommand failed.

I'm also wondering if just transform_iterator would be more fitting name, since this doesn't seem to be io related.

Thanks for the patch (esp the tests and examples!), I'll take a closer look through it next week.

[trevorsm7]

Thanks, I'll fix that include next week.

I wasn't sure what to call this either. transform_iterator and transform_output_iterator are taken already unfortunately. Is there a better nomenclature for iterators that are both InputIterator and OutputIterator?

[alliepiper]

Ah, yeah, that's a good reason :-)

IMO, ideally we'd make transform_iterator into an input/output iterator, but with the existing template parameters on that class it'd be tough to squeeze in. Not to bikeshed this too much, but I'm not a fan of io, since it sounds like it's related to file/network IO. I've used the term invertible_transform in similar contexts before, maybe there's something there?

Let's think about it over the weekend and see if something more suitable comes to mind. @jaredhoberock @griwes @brycelelbach Any ideas?

[trevorsm7]

Oh, something like invertible_transform_iterator sounds good to me!

[trevorsm7]

There's a completely separate definition of transform_output_iterator in thrust/system/cuda/detail/util.h. What is this for and would something similar need to be done for this PR?
https://github.com/thrust/thrust/blob/e4782430103201ae844fdaf1a41f33f53343cab3/thrust/system/cuda/detail/util.h#L610-L726

[brycelelbach]

I think I prefer a name of the form transform_X_iterator instead of say invertible_transform_iterator.

@jaredhoberock what do you think?

[trevorsm7]

How about transform_invertible_iterator?

[jaredhoberock]

inout_transform_iterator? It doesn't seem like this feature is related in any way to the invertibility of a function.

[jaredhoberock]

I'd suggest avoiding leading names like ForwardTransform and ReverseTransform. There's no mechanism that can enforce the idea that these functions are inverses of each other. As far as I can tell, the two functions needn't be related at all, so we shouldn't suggest that they must be.

[jaredhoberock]

transform_input_output_iterator, in concert with transform_output_iterator, would also be OK with me. Yeah, the name is long, but this is a somewhat odd iterator that won't be in common use.

I agree with @allisonvacanti that IO sounds like it has something to do with file or network activity.

[trevorsm7]

I'd suggest avoiding leading names like ForwardTransform and ReverseTransform. There's no mechanism that can enforce the idea that these functions are inverses of each other. As far as I can tell, the two functions needn't be related at all, so we shouldn't suggest that they must be.

I can rename the typenames UnaryFunction1 and UnaryFunction2 if that's preferred. How about input_transform and output_transform for the param names?

[jaredhoberock]

UnaryFunction1 and UnaryFunction2 would be conventional, however I think InputFunction and OutputFunction would be even better.

If you go with InputFunction and OutputFunction, I would suggest snake casing those names to input_function and output_function to create the parameter names.

[trevorsm7]

Renamed to transform_input_output_iterator along with the suggestions for the parameter names above.

[alliepiper]

There's a completely separate definition of transform_output_iterator in thrust/system/cuda/detail/util.h. What is this for and would something similar need to be done for this PR?

It looks like that's an implementation detail for the CUDA backend, and it doesn't look like anything actually uses it anymore. I think we can ignore it.

[alliepiper]

Some minor style stuff pointed out inline, but otherwise this LGTM.

@brycelelbach @jaredhoberock, anything else?

[alliepiper]

Also -- @trevorsm7 Can you squash this all down into a single commit? That'll make it easier on our end to test and integrate.

[jaredhoberock]

It looks like that's an implementation detail for the CUDA backend, and it doesn't look like anything actually uses it anymore. I think we can ignore it.

If that iterator code is truly not in use, then it should be eliminated immediately. Can we do that as part of this PR?

[trevorsm7]

It looks like that's an implementation detail for the CUDA backend, and it doesn't look like anything actually uses it anymore. I think we can ignore it.

If that iterator code is truly not in use, then it should be eliminated immediately. Can we do that as part of this PR?

@jaredhoberock @allisonvacanti There are several such definitions in that file e.g. transform_input_iterator_t, static_integer_iterator, counting_iterator_t. Is all of that also unused?
https://github.com/thrust/thrust/blob/e4782430103201ae844fdaf1a41f33f53343cab3/thrust/system/cuda/detail/util.h

[jaredhoberock]

@trevorsm7 The _t naming is unconventional to Thrust. I don't recognize those types. They may have been introduced as part of an older CUB backend that is no longer in use.

[trevorsm7]

It looks like transform_triple_of_input_iterators_t and static_integer_iterator (no _t 😄) are unused as well, but the other _t iterators are used internally within the cuda_cub namespace. I wonder if the ones that are in use could be replaced with the public ones under thrust/iterator though?

[jaredhoberock]

@trevorsm7 Could be.

To get you unblocked, I'd recommend that we merge this PR (assuming there are no other outstanding issues) and deal with the superfluous iterators in a separate PR.

[alliepiper]

I agree, let's do the cleanup in a followup PR. I created issue #1187 to track this.

It looks like this is ready to go, I'll submit this to our internal CI and start integrating. Thanks for the patch and the quick responses, @trevorsm7!

[alliepiper]

CI running in CL 28521053

[alliepiper]

Last push adds an explicitly default copy constructor to the reference proxy. This fixes an issue found by CI:

/dvs/p4/build/sw/gpgpu/thrust/thrust/iterator/detail/transform_input_output_iterator.inl:55:9: error: implicitly-declared 'constexpr thrust::detail::transform_input_output_iterator_proxy<thrust::negate<signed char>, thrust::square<signed char>, thrust::detail::normal_iterator<signed char*> >::transform_input_output_iterator_proxy(const thrust::detail::transform_input_output_iterator_proxy<thrust::negate<signed char>, thrust::square<signed char>, thrust::detail::normal_iterator<signed char*> >&)' is deprecated [-Werror=deprecated-copy]
   55 |       return *this;
      |         ^~~~
/dvs/p4/build/sw/gpgpu/thrust/thrust/iterator/detail/transform_input_output_iterator.inl:60:39: note: because 'thrust::detail::transform_input_output_iterator_proxy<thrust::negate<signed char>, thrust::square<signed char>, thrust::detail::normal_iterator<signed char*> >' has user-provided 'thrust::detail::transform_input_output_iterator_proxy<InputFunction, OutputFunction, Iterator> thrust::detail::transform_input_output_iterator_proxy<InputFunction, OutputFunction, Iterator>::operator=(const thrust::detail::transform_input_output_iterator_proxy<InputFunction, OutputFunction, Iterator>&) [with InputFunction = thrust::negate<signed char>; OutputFunction = thrust::square<signed char>; Iterator = thrust::detail::normal_iterator<signed char*>]'
   60 |     transform_input_output_iterator_proxy operator=(const transform_input_output_iterator_proxy& x)
      |                                       ^~~~~~~~

Rerunning CI.

[alliepiper]

TIL we validate example outputs on internal CI.

Rebased and added FileCheck reference for new example -- rerunning CI.

[alliepiper]

Tests are looking good, merging this soon. Thanks for the patch!