Skip to content

Change nvtext::load_vocabulary_file to return a unique ptr - #7424

Merged
rapids-bot[bot] merged 2 commits into
NVIDIA:branch-0.19from
davidwendt:hashed-vocab-ptr
Feb 24, 2021
Merged

Change nvtext::load_vocabulary_file to return a unique ptr#7424
rapids-bot[bot] merged 2 commits into
NVIDIA:branch-0.19from
davidwendt:hashed-vocab-ptr

Conversation

@davidwendt

@davidwendt davidwendt commented Feb 22, 2021

Copy link
Copy Markdown
Contributor

Reference #5868

This PR changes the nvtext::load_vocabulary_file to return a unique-pointer to make it easier to manage in Python/Cython class object. The original signature returned a flat structure that contained unique-pointers which would make it difficult to copy and manage.

The corresponding gtests and gbenchmarks were updated for this API change.

@davidwendt davidwendt added 3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code. strings strings issues (C++ and Python) improvement Improvement / enhancement to an existing function breaking Breaking change labels Feb 22, 2021
@davidwendt davidwendt self-assigned this Feb 22, 2021
@davidwendt
davidwendt requested a review from a team as a code owner February 22, 2021 22:15

@jrhemstad jrhemstad 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.

This is a code smell. There is rarely a reason to return a shared_ptr from a function. It should likely return a unique_ptr and if someone desires a shared_ptr they can trivially make it into one.

@kkraus14

Copy link
Copy Markdown
Contributor

This is a code smell. There is rarely a reason to return a shared_ptr from a function. It should likely return a unique_ptr and if someone desires a shared_ptr they can trivially make it into one.

Just commenting that from the Python side we can handle a unique_ptr or a shared_ptr so defer to libcudf semantics here.

@davidwendt davidwendt changed the title Change nvtext::load_vocabulary_file to return shared ptr Change nvtext::load_vocabulary_file to return a unique ptr Feb 22, 2021
@codecov

codecov Bot commented Feb 23, 2021

Copy link
Copy Markdown

Codecov Report

❗ No coverage uploaded for pull request base (branch-0.19@b887e58). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff               @@
##             branch-0.19    #7424   +/-   ##
==============================================
  Coverage               ?   82.21%           
==============================================
  Files                  ?      101           
  Lines                  ?    17057           
  Branches               ?        0           
==============================================
  Hits                   ?    14023           
  Misses                 ?     3034           
  Partials               ?        0           

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 b887e58...6efbf6e. Read the comment docs.

@codereport codereport 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.

lgtm

@davidwendt

Copy link
Copy Markdown
Contributor Author

@gpucibot merge

@rapids-bot
rapids-bot Bot merged commit b0e5aef into NVIDIA:branch-0.19 Feb 24, 2021
@davidwendt
davidwendt deleted the hashed-vocab-ptr branch February 24, 2021 13:27
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 breaking Breaking change improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. strings strings issues (C++ and Python)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants