Skip to content

Refactor string conversion check - #7599

Merged
rapids-bot[bot] merged 13 commits into
NVIDIA:branch-0.19from
ttnghia:refactor_string_conversion_check
Mar 17, 2021
Merged

Refactor string conversion check#7599
rapids-bot[bot] merged 13 commits into
NVIDIA:branch-0.19from
ttnghia:refactor_string_conversion_check

Conversation

@ttnghia

@ttnghia ttnghia commented Mar 15, 2021

Copy link
Copy Markdown
Contributor

This addresses #7557.

In summary:

  • Move cudf::strings::is_integer() code from strings/chars_types.* to strings/convert/convert_integers.hpp/cu
  • Move cudf::strings::is_float() code from strings/chars_types.* to strings/convert/convert_floats.hpp/cu
  • Remove cudf::strings::all_integer() and cudf::strings::all_float()

@ttnghia
ttnghia requested a review from a team as a code owner March 15, 2021 15:56
@ttnghia
ttnghia requested review from jrhemstad and trxcllnt March 15, 2021 15:56
@ttnghia ttnghia linked an issue Mar 15, 2021 that may be closed by this pull request
[FEA] Refactor string conversion check #7557
Closed
@github-actions github-actions Bot added the libcudf Affects libcudf (C++/CUDA) code. label Mar 15, 2021
@ttnghia
ttnghia requested a review from davidwendt March 15, 2021 15:57
@ttnghia ttnghia added 3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function libcudf blocker non-breaking Non-breaking change strings strings issues (C++ and Python) and removed libcudf blocker labels Mar 15, 2021
@ttnghia ttnghia mentioned this pull request Mar 15, 2021
Closed
@davidwendt

davidwendt commented Mar 15, 2021

Copy link
Copy Markdown
Contributor

You will have to change the cython/python interfaces to these too or this will not build.

davidwendt
davidwendt reviewed Mar 15, 2021
View reviewed changes
Comment thread cpp/tests/strings/chars_types_tests.cpp Outdated
@ttnghia
ttnghia requested review from a team as code owners March 15, 2021 17:32
@ttnghia
ttnghia requested a review from shwina March 15, 2021 17:32
@ttnghia

ttnghia commented Mar 15, 2021

Copy link
Copy Markdown
Contributor Author

You will have to change the cython/python interfaces to these too or this will not build.

Done.

@ttnghia ttnghia added Python Affects Python cuDF API. Cython and removed Python Affects Python cuDF API. labels Mar 15, 2021
@ttnghia

ttnghia commented Mar 15, 2021

Copy link
Copy Markdown
Contributor Author

Rerun tests.

galipremsagar
galipremsagar approved these changes Mar 15, 2021
View reviewed changes

@galipremsagar galipremsagar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

cython/python refactor LGTM

jlowe
jlowe approved these changes Mar 15, 2021
View reviewed changes

@jlowe jlowe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Java approval

@ttnghia

ttnghia commented Mar 15, 2021

Copy link
Copy Markdown
Contributor Author

Rerun tests.

@github-actions github-actions Bot added Java Affects Java cuDF API. Python Affects Python cuDF API. labels Mar 16, 2021
@ttnghia

ttnghia commented Mar 16, 2021

Copy link
Copy Markdown
Contributor Author

Rerun tests.

@codecov

codecov Bot commented Mar 16, 2021 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #7599 (b7e2844) into branch-0.19 (7871e7a) will increase coverage by 0.52%.
The diff coverage is 93.84%.

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

@@               Coverage Diff               @@
##           branch-0.19    #7599      +/-   ##
===============================================
+ Coverage        81.86%   82.39%   +0.52%     
===============================================
  Files              101      101              
  Lines            16884    17352     +468     
===============================================
+ Hits             13822    14297     +475     
+ Misses            3062     3055       -7
Impacted Files Coverage Δ
python/cudf/cudf/core/index.py 93.34% <ø> (+0.48%) ⬆️
python/cudf/cudf/core/column/numerical.py 94.83% <87.50%> (-0.20%) ⬇️
python/cudf/cudf/core/frame.py 89.09% <89.47%> (+0.08%) ⬆️
python/cudf/cudf/core/column/column.py 87.86% <90.00%> (+0.10%) ⬆️
python/cudf/cudf/core/column/decimal.py 92.75% <90.32%> (-2.12%) ⬇️
python/cudf/cudf/core/dataframe.py 90.58% <95.65%> (+0.11%) ⬆️
python/cudf/cudf/core/series.py 91.57% <95.83%> (+0.79%) ⬆️
python/cudf/cudf/core/column/categorical.py 91.97% <100.00%> (+0.58%) ⬆️
python/cudf/cudf/core/column/datetime.py 89.63% <100.00%> (+0.54%) ⬆️
python/cudf/cudf/core/column/string.py 86.79% <100.00%> (+0.30%) ⬆️
... and 59 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 325d5b8...dae6d37. Read the comment docs.

@ttnghia
ttnghia requested a review from davidwendt March 16, 2021 17:13
@ttnghia

ttnghia commented Mar 16, 2021

Copy link
Copy Markdown
Contributor Author

Rerun tests.

@ttnghia

ttnghia commented Mar 16, 2021

Copy link
Copy Markdown
Contributor Author

Rerun tests.

1 similar comment
@ttnghia

ttnghia commented Mar 17, 2021

Copy link
Copy Markdown
Contributor Author

Rerun tests.

davidwendt
davidwendt requested changes Mar 17, 2021
View reviewed changes

@davidwendt davidwendt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you update the copyright years for these files?

@ttnghia

ttnghia commented Mar 17, 2021

Copy link
Copy Markdown
Contributor Author

Can you update the copyright years for these files?

Sorry I forgot that again. Just updated.

@ttnghia

ttnghia commented Mar 17, 2021

Copy link
Copy Markdown
Contributor Author

@gpucibot merge

@rapids-bot
rapids-bot Bot merged commit 3349764 into NVIDIA:branch-0.19 Mar 17, 2021
@ttnghia ttnghia self-assigned this Apr 25, 2021
@ttnghia
ttnghia deleted the refactor_string_conversion_check branch May 3, 2021 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@galipremsagar galipremsagar galipremsagar approved these changes
@jrhemstad jrhemstad jrhemstad approved these changes
@davidwendt davidwendt davidwendt approved these changes
@trxcllnt trxcllnt Awaiting requested review from trxcllnt
@shwina shwina Awaiting requested review from shwina
+1 more reviewer
@jlowe jlowe jlowe approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@ttnghia ttnghia

Labels

3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function Java Affects Java cuDF API. libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change Python Affects Python cuDF API. strings strings issues (C++ and Python)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[FEA] Refactor string conversion check

5 participants

@ttnghia @davidwendt @jlowe @galipremsagar @jrhemstad