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

[REVIEW] Resolve unnecessary import of thrust/optional.hpp in types.hpp #7667

Merged
merged 4 commits into from
Mar 23, 2021
Merged

[REVIEW] Resolve unnecessary import of thrust/optional.hpp in types.hpp #7667

merged 4 commits into from
Mar 23, 2021

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Mar 22, 2021

Resolves #6475. The missing required include of optional.hpp was in convert_datetime.cu.

@vyasr vyasr requested a review from a team as a code owner March 22, 2021 14:48
@vyasr vyasr requested review from harrism and cwharris March 22, 2021 14:48
@vyasr vyasr changed the title Resolve unnecessary import of thrust/optional.hpp in types.hpp [REVIEW] Resolve unnecessary import of thrust/optional.hpp in types.hpp Mar 22, 2021
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Mar 22, 2021
@vyasr vyasr requested a review from codereport March 22, 2021 14:48
@vyasr vyasr added 3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond improvement Improvement / enhancement to an existing function non-breaking Non-breaking change tech debt labels Mar 22, 2021
@vyasr vyasr mentioned this pull request Mar 22, 2021
Closed
codereport
codereport approved these changes Mar 22, 2021
View reviewed changes
Copy link
Contributor

@codereport codereport left a comment

Choose a reason for hiding this comment

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

🔥 🔥 🔥 🔥

@vyasr
Copy link
Contributor Author

vyasr commented Mar 22, 2021

My local repo wasn't up-to-date, looks like #7461 introduced additional usage of optional relying on the include in types.hpp. Fixing now.

@vyasr
Copy link
Contributor Author

vyasr commented Mar 22, 2021

rerun tests

harrism
harrism approved these changes Mar 23, 2021
View reviewed changes
Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

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

Nice find! Just one copyright needs updated (types.hpp)

cpp/include/cudf/types.hpp Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 23, 2021 •
edited
Loading

Codecov Report

Merging #7667 (6281fa7) into branch-0.19 (7871e7a) will increase coverage by 0.62%.
The diff coverage is n/a.

❗ Current head 6281fa7 differs from pull request most recent head 6b26871. Consider uploading reports for the commit 6b26871 to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.19    #7667      +/-   ##
===============================================
+ Coverage        81.86%   82.49%   +0.62%     
===============================================
  Files              101      101              
  Lines            16884    17400     +516     
===============================================
+ Hits             13822    14354     +532     
+ Misses            3062     3046      -16
Impacted Files Coverage Δ
python/cudf/cudf/core/column/categorical.py 91.97% <ø> (+0.58%) ⬆️
python/cudf/cudf/core/column/column.py 87.86% <ø> (+0.10%) ⬆️
python/cudf/cudf/core/column/datetime.py 89.63% <ø> (+0.54%) ⬆️
python/cudf/cudf/core/column/decimal.py 92.75% <ø> (-2.12%) ⬇️
python/cudf/cudf/core/column/lists.py 92.17% <ø> (+0.77%) ⬆️
python/cudf/cudf/core/column/numerical.py 94.83% <ø> (-0.20%) ⬇️
python/cudf/cudf/core/column/string.py 86.79% <ø> (+0.30%) ⬆️
python/cudf/cudf/core/column/timedelta.py 88.57% <ø> (+0.33%) ⬆️
python/cudf/cudf/core/column_accessor.py 95.45% <ø> (+0.14%) ⬆️
python/cudf/cudf/core/dataframe.py 90.90% <ø> (+0.44%) ⬆️
... and 61 more

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 c21bd0e...6b26871. Read the comment docs.

@harrism harrism added 0 - Waiting on Author Waiting for author to respond to review and removed 3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond labels Mar 23, 2021
@codereport codereport added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 0 - Waiting on Author Waiting for author to respond to review labels Mar 23, 2021
@codereport
Copy link
Contributor

@gpucibot merge

@rapids-bot rapids-bot bot merged commit d500142 into rapidsai:branch-0.19 Mar 23, 2021
@vyasr vyasr deleted the fix/issue_6475 branch January 14, 2022 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codereport codereport codereport approved these changes

@jrhemstad jrhemstad jrhemstad approved these changes

@harrism harrism harrism approved these changes

@cwharris cwharris Awaiting requested review from cwharris

Assignees
No one assigned
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.

[FEA] Remove #include <thrust/optional.h> from types.hpp
4 participants
@vyasr @codereport @harrism @jrhemstad