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

Add zip_function adaptor to simplify using n-ary functions with zip iterators #1029

Merged
merged 1 commit into from
Mar 4, 2020
Merged

Add zip_function adaptor to simplify using n-ary functions with zip iterators #1029

merged 1 commit into from
Mar 4, 2020

Conversation

bjude
Copy link
Contributor

@bjude bjude commented Oct 26, 2019

#721 is stale and hasn't been updated in a few years so this PR updates it and adds a test

@brycelelbach
Copy link
Collaborator

Running internal CI.

@griwes and @jaredhoberock for code review.

@bjude
Copy link
Contributor Author

bjude commented Oct 28, 2019

Note that i've left the two new files without a copyright message in them for now as the style seems to be slightly different between the different files I had a quick look at for guidance and i wasnt sure exactly whats needed. Let me know what format you want it in and i can add it or a reviewer can add in whatever is needed by nvidia

@jaredhoberock
Copy link
Contributor

Great submission!

jaredhoberock
jaredhoberock reviewed Oct 28, 2019
View reviewed changes
Copy link
Contributor

@jaredhoberock jaredhoberock left a comment

Choose a reason for hiding this comment

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

Looks pretty good. Aside from the small nitpicks, this looks ready to merge as long as we get documentation. Ideally, that would be:

  • Doxygen-style comments for the public functionality
  • An example program

thrust/zip_function.h Show resolved Hide resolved
thrust/zip_function.h Show resolved Hide resolved
thrust/zip_function.h Outdated
}
}

// Adaptor type that turns an N-ary function object into one that takes a tuple
Copy link
Contributor

Choose a reason for hiding this comment

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

This class a Doxygen-style comment, preferably with a demo code snippet.

thrust/zip_function.h Show resolved Hide resolved
thrust/zip_function.h Show resolved Hide resolved
thrust/zip_function.h Outdated Show resolved Hide resolved
thrust/zip_function.h Outdated Show resolved Hide resolved
brycelelbach
brycelelbach suggested changes Oct 28, 2019
View reviewed changes
Copy link
Collaborator

@brycelelbach brycelelbach left a comment

Choose a reason for hiding this comment

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

This is C++11 only, so please give it a C++11 only guard/warning.

thrust/zip_function.h Show resolved Hide resolved
@bjude
Copy link
Contributor Author

bjude commented Feb 20, 2020

Any updates on this review? i believe i've covered off the requested changes.
The only thing i see thats still missing is a copyright notice, i'm guessing it will need something like:

/*
 *  Copyright 2020 NVIDIA Corporation
 *
 *  Licensed under the Apache License, Version 2.0 (the "License");
 *  you may not use this file except in compliance with the License.
 *  You may obtain a copy of the License at
 *
 *      http://www.apache.org/licenses/LICENSE-2.0
 *
 *  Unless required by applicable law or agreed to in writing, software
 *  distributed under the License is distributed on an "AS IS" BASIS,
 *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 *  See the License for the specific language governing permissions and
 *  limitations under the License.
 */

@brycelelbach @griwes @jaredhoberock can you confirm if the above license is adequate? I'm happy to sign the code over to NVIDIA

jaredhoberock
jaredhoberock approved these changes Feb 20, 2020
View reviewed changes
Copy link
Contributor

@jaredhoberock jaredhoberock left a comment

Choose a reason for hiding this comment

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

I think this is almost ready to go -- I had requested an example program and Doxygen comments, and they're in place.

The final thing to do is to update the CHANGELOG to mention these new features and credit @bjude.

alliepiper
alliepiper reviewed Feb 21, 2020
View reviewed changes
thrust/zip_function.h Outdated Show resolved Hide resolved
@alliepiper
Copy link
Collaborator

I agree that this looks like a really useful feature! I just noted a couple small issues to address before we merge.

@alliepiper
Copy link
Collaborator

Looks like this is almost ready to go. If you could add the missing newline at the end of zip_function.h I'll get this merged soon :-)

@bjude
Copy link
Contributor Author

bjude commented Mar 3, 2020

Missing newline is added.
Looks like the PR is blocked on @brycelelbach s request for a c++11 guard which was added in a6d668e

@alliepiper
Copy link
Collaborator

@bjude Just to keep you in the loop, I'm working on getting this in now. There are some NVIDIA-internal steps I need to go through to get this merged, but it shouldn't be much longer.

@alliepiper
Copy link
Collaborator

Internal CI submitted under CL 28074591.

@bjude @alliepiper
Add zip_function to adapt N-ary functions to take a tuple
0387506 
Eases the use general function objects with zip iterators without modifying them or hand writing a wrapping class

Test for zip_function

Based on the zip iterator transform test

zip_function: Move details into thrust::detal::zip_detail

zip_function: make operator() const and make stored function mutable

CMake: Add filter for test that require c++11

Only add zip_function for now, making the list exhaustive can be another PR

zip_function: Add example to arbitrary_transformation

zip_function: Add c++11 guard

zip_function: Documentation

Zip Function: newline at end of file

Allison rewrote some bits to support C++11 compilers.

Reviewed-by: Allison Vacanti <alliepiper16@gmail.com>
@alliepiper
Copy link
Collaborator

Rebased, squashed, and fixed some c++11-specific issues caught by CI.

@alliepiper alliepiper merged commit 82370c2 into NVIDIA:master Mar 4, 2020
@alliepiper alliepiper mentioned this pull request Mar 4, 2020
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alliepiper alliepiper alliepiper left review comments

@brycelelbach brycelelbach brycelelbach approved these changes

@jaredhoberock jaredhoberock jaredhoberock approved these changes

@griwes griwes Awaiting requested review from griwes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bjude @brycelelbach @jaredhoberock @alliepiper