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

Add view_concat for edge_minor_property_view_t and update transform_reduce_e_by_dst_key to support reduce_op on tuple types #2879

Merged
merged 4 commits into from
Nov 3, 2022

Conversation

naimnv
Copy link
Contributor

@naimnv naimnv commented Nov 2, 2022

…educe_e_by_dst_key to support reduce_op on tuple data types
@naimnv naimnv requested a review from a team as a code owner November 2, 2022 16:13
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.

Looks good to me except for some minor cosmetic issues.

Have you tried to compile this?

@@ -66,7 +66,7 @@ __device__ void update_buffer_element(
EdgePartitionSrcDstKeyInputWrapper edge_partition_src_dst_key_input,
EdgeOp e_op,
typename GraphViewType::vertex_type* key,
T* value)
ValueIterator value_first)
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit of nitpicking,

but in our naming convention, value_first implies the first element of a multi-element array. But here, value_first is actually a reference to the one buffer element we need to update. So, better rename this to something like just value or value_iter.

Copy link
Contributor Author

@naimnv naimnv Nov 2, 2022

Choose a reason for hiding this comment

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

Renamed.
@seunghwak Yes, it compiles locally.

@seunghwak seunghwak added 3 - Ready for Review improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Nov 2, 2022
@seunghwak
Copy link
Contributor

And I assumed this is ready-for-review.

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.12@5364f6f). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@               Coverage Diff               @@
##             branch-22.12    #2879   +/-   ##
===============================================
  Coverage                ?   62.61%           
===============================================
  Files                   ?      114           
  Lines                   ?     6395           
  Branches                ?        0           
===============================================
  Hits                    ?     4004           
  Misses                  ?     2391           
  Partials                ?        0           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ChuckHastings
Copy link
Collaborator

@gpucibot merge

@rapids-bot rapids-bot bot merged commit f16deb5 into rapidsai:branch-22.12 Nov 3, 2022
@naimnv naimnv deleted the add-and-upate-prims-#2878 branch April 4, 2023 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
5 participants