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] string conversion for duration types (to_durations, from_durations) #5625

Merged
merged 63 commits into from
Aug 7, 2020
Merged

[REVIEW] string conversion for duration types (to_durations, from_durations) #5625

merged 63 commits into from
Aug 7, 2020

Conversation

karthikeyann
Copy link
Contributor

@karthikeyann karthikeyann commented Jul 2, 2020 •
edited
Loading

addresses part of #5272
duration string support to_string API

  • from_durations
  • to_durations

@karthikeyann karthikeyann added 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. strings strings issues (C++ and Python) labels Jul 2, 2020
@GPUtester
Copy link
Collaborator

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

@karthikeyann karthikeyann changed the title [WIP] string conversion for duration types (to, from) [WIP] string conversion for duration types (to_durations, from_durations) Jul 8, 2020
@harrism harrism mentioned this pull request Jul 8, 2020
Closed
15 tasks
@codecov
Copy link

codecov bot commented Jul 8, 2020 •
edited
Loading

Codecov Report

Merging #5625 into branch-0.15 will increase coverage by 0.01%.
The diff coverage is 93.75%.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.15    #5625      +/-   ##
===============================================
+ Coverage        86.12%   86.14%   +0.01%     
===============================================
  Files               76       75       -1     
  Lines            12757    12715      -42     
===============================================
- Hits             10987    10953      -34     
+ Misses            1770     1762       -8
Impacted Files Coverage Δ
python/dask_cudf/dask_cudf/backends.py 89.28% <50.00%> (-1.83%) ⬇️
python/cudf/cudf/core/dataframe.py 89.37% <82.35%> (+<0.01%) ⬆️
python/cudf/cudf/core/groupby/groupby.py 93.02% <86.66%> (-0.50%) ⬇️
python/cudf/cudf/core/frame.py 89.72% <87.50%> (-0.19%) ⬇️
python/cudf/cudf/core/series.py 90.92% <94.73%> (-0.13%) ⬇️
python/cudf/cudf/core/column/categorical.py 93.42% <100.00%> (-0.58%) ⬇️
python/cudf/cudf/core/column/column.py 88.63% <100.00%> (+0.03%) ⬆️
python/cudf/cudf/core/column/datetime.py 84.31% <100.00%> (-0.11%) ⬇️
python/cudf/cudf/core/column/lists.py 97.77% <100.00%> (ø)
python/cudf/cudf/core/column/numerical.py 95.25% <100.00%> (+0.91%) ⬆️
... and 17 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 e63eb31...ef93d42. Read the comment docs.

@karthikeyann karthikeyann marked this pull request as ready for review July 8, 2020 15:52
@karthikeyann karthikeyann requested a review from a team as a code owner July 8, 2020 15:52
@karthikeyann karthikeyann added 3 - Ready for Review Ready for review by team and removed 0 - Waiting on Author Waiting for author to respond to review labels Jul 27, 2020
@karthikeyann karthikeyann requested a review from davidwendt July 28, 2020 01:48
@karthikeyann karthikeyann requested a review from davidwendt July 28, 2020 12:31
davidwendt
davidwendt approved these changes Jul 28, 2020
View reviewed changes
Copy link
Contributor

@davidwendt davidwendt left a comment

Choose a reason for hiding this comment

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

Nice job on this. I liked these improvements over what's in the current convert_integers code.

@karthikeyann karthikeyann added the 4 - Needs Review Waiting for reviewer to review or respond label Jul 28, 2020
harrism
harrism requested changes Jul 30, 2020
View reviewed changes
cpp/benchmarks/CMakeLists.txt Outdated Show resolved Hide resolved
cpp/include/cudf/strings/convert/convert_durations.hpp Outdated Show resolved Hide resolved
cpp/src/strings/convert/convert_durations.cu Show resolved Hide resolved
cpp/src/strings/convert/convert_durations.cu
}
bool is_negative = (value < 0);

char digits[MAX_DIGITS] = {'0', '0', '0', '0', '0', '0', '0', '0', '0', '0'};
Copy link
Member

Choose a reason for hiding this comment

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

If this becomes a problem, profile it and see if putting it in shared memory (10 bytes per thread, not actually shared) helps performance. Might need to pad it out to avoid shmem bank conflicts.

cpp/tests/utilities/column_utilities.hpp Show resolved Hide resolved
@galipremsagar galipremsagar mentioned this pull request Aug 3, 2020
Merged
17 tasks
@karthikeyann
Copy link
Contributor Author

@codereport @jrhemstad requesting for re-review

codereport
codereport approved these changes Aug 6, 2020
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.

👍

cpp/src/strings/convert/convert_durations.cu Show resolved Hide resolved
@karthikeyann karthikeyann added the 5 - Ready to Merge Testing and reviews complete, ready to merge label Aug 7, 2020
@karthikeyann karthikeyann dismissed jrhemstad’s stale review August 7, 2020 07:53

for unblocking PR #5781 (also already addressed all review comments)

@karthikeyann karthikeyann merged commit 2f69ced into rapidsai:branch-0.15 Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidwendt davidwendt davidwendt approved these changes

@codereport codereport codereport approved these changes

@raydouglass raydouglass raydouglass approved these changes

@harrism harrism harrism approved these changes

@jrhemstad jrhemstad Awaiting requested review from jrhemstad

@kkraus14 kkraus14 Awaiting requested review from kkraus14

Assignees

@karthikeyann karthikeyann

Labels
3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond 5 - Ready to Merge Testing and reviews complete, ready to merge libcudf Affects libcudf (C++/CUDA) code. strings strings issues (C++ and Python)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@karthikeyann @GPUtester @harrism @raydouglass @kkraus14 @jrhemstad @codereport @davidwendt