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

Mods for pylibcugraph #2030

Closed
wants to merge 14 commits into from
Closed

Mods for pylibcugraph #2030

wants to merge 14 commits into from

Conversation

ChuckHastings
Copy link
Collaborator

@ChuckHastings ChuckHastings commented Jan 22, 2022 •
edited
Loading

Changes to support #2023

  • Adds view semantic to the cugraph_type_erased_device_array
  • Add device to device copy

Should not be merged until #2023 has been tested with it.

UPDATE: The commits from this PR were merged into #2023. So once 2023 is merged then all of these changes will be in the baseline. I will close this PR without merging once 2023 is merged.

@ChuckHastings ChuckHastings requested a review from a team as a code owner January 22, 2022 01:31
@rlratzel rlratzel added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jan 22, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jan 22, 2022 •
edited
Loading

Codecov Report

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

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

@@               Coverage Diff               @@
##             branch-22.02    #2030   +/-   ##
===============================================
  Coverage                ?   70.75%           
===============================================
  Files                   ?      142           
  Lines                   ?     8861           
  Branches                ?        0           
===============================================
  Hits                    ?     6270           
  Misses                  ?     2591           
  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 4032f34...720ffa5. Read the comment docs.

@BradReesWork BradReesWork added this to the 22.02 milestone Jan 24, 2022
@ChuckHastings ChuckHastings self-assigned this Jan 24, 2022
@ChuckHastings ChuckHastings added the DO NOT MERGE Hold off on merging; see PR for details label Jan 24, 2022
@BradReesWork BradReesWork removed the DO NOT MERGE Hold off on merging; see PR for details label Jan 25, 2022
@BradReesWork BradReesWork added DO NOT MERGE Hold off on merging; see PR for details 5 - Merge After Dependencies labels Jan 25, 2022
seunghwak
seunghwak approved these changes Jan 25, 2022
View reviewed changes
Copy link
Contributor

@seunghwak seunghwak left a comment

Choose a reason for hiding this comment

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

LGTM

cpp/include/cugraph_c/array.h
@@ -55,44 +63,87 @@ cugraph_error_code_t cugraph_type_erased_device_array_create(
*/
void cugraph_type_erased_device_array_free(cugraph_type_erased_device_array_t* p);

#if 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Better delete if no longer necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a FIXME in the latest push. This was going to be part of the API, but it can't currently be implemented. We need to have a discussion about whether we can implement this or whether we should address this a different way.

cpp/include/cugraph_c/array.h
@@ -103,43 +154,84 @@ cugraph_error_code_t cugraph_type_erased_host_array_create(const cugraph_resourc
*/
void cugraph_type_erased_host_array_free(cugraph_type_erased_host_array_t* p);

#if 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Better delete if no longer necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See above comment

@ChuckHastings ChuckHastings removed DO NOT MERGE Hold off on merging; see PR for details 5 - Merge After Dependencies labels Jan 26, 2022
@ChuckHastings ChuckHastings added the DO NOT MERGE Hold off on merging; see PR for details label Jan 26, 2022
@BradReesWork BradReesWork removed the DO NOT MERGE Hold off on merging; see PR for details label Jan 26, 2022
@ChuckHastings ChuckHastings added the DO NOT MERGE Hold off on merging; see PR for details label Jan 26, 2022
@BradReesWork
Copy link
Member

fixes rolled into #2023

@ChuckHastings ChuckHastings deleted the mods_for_pylibcugraph branch February 1, 2022 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seunghwak seunghwak seunghwak approved these changes

@kaatish kaatish Awaiting requested review from kaatish

Assignees

@ChuckHastings ChuckHastings

Labels
DO NOT MERGE Hold off on merging; see PR for details improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Milestone
22.02
Development

Successfully merging this pull request may close these issues.
5 participants
@ChuckHastings @codecov-commenter @BradReesWork @seunghwak @rlratzel