Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move cudf::test::make_counting_transform_iterator to cudf/detail/iterator.cuh #7306

Merged
merged 7 commits into from
Feb 5, 2021
Merged

Move cudf::test::make_counting_transform_iterator to cudf/detail/iterator.cuh #7306

merged 7 commits into from
Feb 5, 2021

Conversation

codereport
Copy link
Contributor

@codereport codereport commented Feb 4, 2021 •
edited
Loading

😭 😭 😭

Oh how fragile cuDF is. Ran into so many issues.

This was split out of #6546 because of how complicated it was.

95% was just:

  • find & replace
  • adding headers

The other 5% was:

  • issues with column_device_view.cuh
  • missing dependent headers

@codereport codereport added libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Feb 4, 2021
@codereport codereport self-assigned this Feb 4, 2021
@codereport codereport marked this pull request as ready for review February 4, 2021 07:24
@codereport codereport requested a review from a team as a code owner February 4, 2021 07:24
@codereport codereport added 3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond labels Feb 4, 2021
@codereport codereport mentioned this pull request Feb 4, 2021
Merged
trxcllnt
trxcllnt approved these changes Feb 4, 2021
View reviewed changes
Copy link
Contributor

@trxcllnt trxcllnt left a comment

Choose a reason for hiding this comment

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

Assuming this passes CI, it looks good to me.

Clang has always complained about the conditional #ifdef __CUDACC__ blocks in bit.hpp, but I assume fixing that is out of scope for this PR.

trxcllnt
trxcllnt reviewed Feb 4, 2021
View reviewed changes
cpp/include/cudf/column/column_device_view.cuh
@@ -592,6 +593,7 @@ class alignas(16) mutable_column_device_view : public detail::column_device_view
return d_children[child_index];
}

#ifdef __CUDACC__ // because set_bit in bit.hpp is wrapped with __CUDACC__
Copy link
Contributor

Choose a reason for hiding this comment

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

These are necessary even though this is a .cuh file?

I thought .cuh files could only be included by .cu files, compiled by nvcc, so __CUDACC__ would always be defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that is the guideline as well. However, this is overwhelming not followed (which I discovered from workin on this PR). For instance, column_wrapper.hpp where the make_counting_transform_iterator function used to be, has a .hpp, and the majority of the test files that used it were also .hpp.

Copy link
Contributor

@trxcllnt trxcllnt Feb 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

So you're saying column_wrapper.hpp or other .cpp/.hpp files are now doing this?

#include <cudf/detail/iterator.cuh>

That seems like a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, but sort of out of scope of this PR. Although ... I just did

cat $(find . | grep "p$") | grep thrust | wc -l

in both cpp/src and cpp/include and the total was 53. However, upon brief review ... most of the references were to thrust::host_vector or thrust::make_**_iterator` so maybe those aren't consider "device" code?

Just did the same grep on cpp/tests and got 533 😮

cat $(find . | grep "p$") | grep __device__ | wc -l

yields 27 across src include and tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@trxcllnt I will set up a issue / PR to rename filenames / look into this.

@codecov
Copy link

codecov bot commented Feb 4, 2021 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (branch-0.19@53d7ad2). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff               @@
##             branch-0.19    #7306   +/-   ##
==============================================
  Coverage               ?   81.80%           
==============================================
  Files                  ?      100           
  Lines                  ?    16606           
  Branches               ?        0           
==============================================
  Hits                   ?    13584           
  Misses                 ?     3022           
  Partials               ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53d7ad2...7d2239d. Read the comment docs.

karthikeyann
karthikeyann requested changes Feb 4, 2021
View reviewed changes
cpp/include/cudf/detail/iterator.cuh Outdated Show resolved Hide resolved
cpp/include/cudf/detail/iterator.cuh Show resolved Hide resolved
cpp/tests/binaryop/binop-fixture.hpp Show resolved Hide resolved
cpp/tests/strings/booleans_tests.cpp Show resolved Hide resolved
cpp/tests/strings/case_tests.cpp Show resolved Hide resolved
cpp/tests/strings/chars_types_tests.cpp Show resolved Hide resolved
cpp/tests/unary/cast_tests.cpp Outdated Show resolved Hide resolved
@codereport codereport changed the title Move cudf::test::make_counting_transform_iterator to cudf/detail/iterator.cuh Move cudf::test::make_counting_transform_iterator to cudf/detail/iterator.cuh [skip ci] Feb 4, 2021
karthikeyann
karthikeyann approved these changes Feb 4, 2021
View reviewed changes
Copy link
Contributor

@karthikeyann karthikeyann left a comment

Choose a reason for hiding this comment

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

LGTM

cpp/include/cudf_test/column_wrapper.hpp Outdated Show resolved Hide resolved
@codereport codereport changed the title Move cudf::test::make_counting_transform_iterator to cudf/detail/iterator.cuh [skip ci] Move cudf::test::make_counting_transform_iterator to cudf/detail/iterator.cuh Feb 4, 2021
@codereport codereport added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond labels Feb 4, 2021
@codereport codereport removed the request for review from devavret February 4, 2021 17:24
@codereport
Copy link
Contributor Author

rerun tests

@galipremsagar
Copy link
Contributor

rerun tests

@codereport
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 8215b5b into rapidsai:branch-0.19 Feb 5, 2021
rapids-bot bot pushed a commit that referenced this pull request Feb 6, 2021
@codereport
Cleaning up for loops with make_(counting_)transform_iterator (#6546
621de88 
)

Doing some cleanup - using `make_counting_transform_iterator` and `make_transform_iterator` to clean up `for` loops and other code.

Making an argument that `make_counting_transform_iterator` should be moved out of `cudf::test::`.

Also, will clean up the `f`/`begin` and `op` naming conventions.

Depends on: #7306

Authors:
  - Conor Hoekstra (@codereport)

Approvers:
  - David (@davidwendt)
  - Jake Hemstad (@jrhemstad)
  - Mark Harris (@harrism)

URL: #6546
@trxcllnt trxcllnt mentioned this pull request Feb 12, 2021
Merged
rapids-bot bot pushed a commit to rapidsai/cuspatial that referenced this pull request Feb 23, 2021
@trxcllnt
Update make_counting_transform_iterator namespace in tests (#353)
d3c3cd8 
Update location of `make_counting_transform_iterator` after rapidsai/cudf#7306.

Authors:
  - Paul Taylor (@trxcllnt)

Approvers:
  - H. Thomson Comer (@thomcom)

URL: #353
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trxcllnt trxcllnt trxcllnt approved these changes

@karthikeyann karthikeyann karthikeyann approved these changes

Assignees

@codereport codereport

Labels
5 - Ready to Merge Testing and reviews complete, ready to merge improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@codereport @galipremsagar @trxcllnt @karthikeyann