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

Add cython for converting strings/fixed-point functions #7429

Merged
merged 16 commits into from
Mar 4, 2021
Merged

Add cython for converting strings/fixed-point functions #7429

merged 16 commits into from
Mar 4, 2021

Conversation

davidwendt
Copy link
Contributor

Reference #7285

This PR adds Cython wrappers for cudf::strings::to_fixed_point, cudf::strings::from_fixed_point, and cudf::strings::is_fixed_point libcudf functions.

@davidwendt davidwendt added feature request New feature or request 3 - Ready for Review Ready for review by team Python Affects Python cuDF API. non-breaking Non-breaking change labels Feb 23, 2021
@davidwendt davidwendt self-assigned this Feb 23, 2021
@davidwendt davidwendt requested a review from shwina February 23, 2021 19:41
@davidwendt davidwendt added 2 - In Progress Currently a work in progress and removed 3 - Ready for Review Ready for review by team labels Feb 23, 2021
@codecov
Copy link

codecov bot commented Feb 23, 2021 •
edited
Loading

Codecov Report

Merging #7429 (9204a7b) into branch-0.19 (43b44e1) will increase coverage by 0.46%.
The diff coverage is 88.37%.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.19    #7429      +/-   ##
===============================================
+ Coverage        81.80%   82.27%   +0.46%     
===============================================
  Files              101      101              
  Lines            16695    17261     +566     
===============================================
+ Hits             13658    14202     +544     
- Misses            3037     3059      +22
Impacted Files Coverage Δ
python/cudf/cudf/utils/docutils.py 97.36% <50.00%> (-2.64%) ⬇️
python/cudf/cudf/testing/testing.py 80.00% <57.14%> (-1.04%) ⬇️
python/cudf/cudf/core/column/timedelta.py 88.57% <75.00%> (+0.33%) ⬆️
python/cudf/cudf/core/column/categorical.py 91.74% <77.14%> (-0.49%) ⬇️
python/cudf/cudf/core/column/datetime.py 89.63% <78.57%> (+0.58%) ⬆️
python/dask_cudf/dask_cudf/core.py 71.67% <79.41%> (-2.60%) ⬇️
python/cudf/cudf/core/multiindex.py 82.90% <90.90%> (+0.73%) ⬆️
python/cudf/cudf/core/tools/datetimes.py 84.53% <93.33%> (+0.89%) ⬆️
python/cudf/cudf/core/column/column.py 87.80% <93.75%> (+0.38%) ⬆️
python/cudf/cudf/core/column_accessor.py 95.47% <95.65%> (+2.53%) ⬆️
... and 66 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 4d0c160...9204a7b. Read the comment docs.

@davidwendt davidwendt marked this pull request as ready for review February 25, 2021 21:23
@davidwendt davidwendt requested a review from a team as a code owner February 25, 2021 21:23
@davidwendt davidwendt added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Feb 25, 2021
davidwendt
davidwendt commented Feb 25, 2021
View reviewed changes
python/cudf/cudf/tests/test_string.py
got = gs.astype(cudf.Decimal64Dtype(scale=-2, precision=5))
assert_eq(fp, got)


Copy link
Contributor Author

Choose a reason for hiding this comment

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

@codereport For input on pytest test cases.

@codereport codereport self-requested a review March 2, 2021 21:57
galipremsagar
galipremsagar requested changes Mar 3, 2021
View reviewed changes
Copy link
Contributor

@galipremsagar galipremsagar left a comment

Choose a reason for hiding this comment

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

Overall looks good, but can we keep the hierarchy of cython functions similar to the cpp counterparts? Like place from_decimal and to_decimal in a new file cudf/_lib/strings/convert/convert_fixed_point.pyx ?

python/cudf/cudf/_lib/string_casting.pyx Outdated Show resolved Hide resolved
python/cudf/cudf/_lib/string_casting.pyx Outdated Show resolved Hide resolved
@davidwendt davidwendt requested a review from galipremsagar March 3, 2021 17:16
shwina
shwina approved these changes Mar 3, 2021
View reviewed changes
Copy link
Contributor

@shwina shwina left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks, David!

@davidwendt
Copy link
Contributor Author

rerun tests

codereport
codereport approved these changes Mar 4, 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.

Tests look fine, we test pretty rigorously on the C++ side.

@davidwendt
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 2dd15b0 into rapidsai:branch-0.19 Mar 4, 2021
@davidwendt davidwendt deleted the fixed-point-to-strings branch March 4, 2021 21:05
@davidwendt davidwendt mentioned this pull request Mar 5, 2021
Closed
hyperbolic2346 pushed a commit to hyperbolic2346/cudf that referenced this pull request Mar 25, 2021
@davidwendt @hyperbolic2346
Add cython for converting strings/fixed-point functions (rapidsai#7429)
d77a393 
Reference rapidsai#7285 

This PR adds Cython wrappers for `cudf::strings::to_fixed_point`, `cudf::strings::from_fixed_point`, and `cudf::strings::is_fixed_point` libcudf functions.

Authors:
  - David (@davidwendt)

Approvers:
  - GALI PREM SAGAR (@galipremsagar)
  - Ashwin Srinath (@shwina)
  - Conor Hoekstra (@codereport)

URL: rapidsai#7429
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

@galipremsagar galipremsagar galipremsagar approved these changes

@shwina shwina shwina approved these changes

Assignees

@davidwendt davidwendt

Labels
3 - Ready for Review Ready for review by team feature request New feature or request non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@davidwendt @shwina @galipremsagar @codereport