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.

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

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

Merged
merged 63 commits into from Aug 7, 2020

Conversation

karthikeyann
Copy link
Contributor

@karthikeyann karthikeyann commented Jul 2, 2020

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
15 tasks
@codecov
Copy link

codecov bot commented Jul 8, 2020

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
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
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
}
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
@karthikeyann
Copy link
Contributor Author

@codereport @jrhemstad requesting for re-review

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)

v0.15 Release automation moved this from PR-Needs review to PR-Reviewer approved Aug 7, 2020
@karthikeyann karthikeyann merged commit 2f69ced into rapidsai:branch-0.15 Aug 7, 2020
v0.15 Release automation moved this from PR-Reviewer approved to Done Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
No open projects
v0.15 Release
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

8 participants