Skip to content

Add is_multigraph to PG and change has_duplicate_edges to use types#2708

Merged
rapids-bot[bot] merged 13 commits intorapidsai:branch-22.10from
eriknw:pg_is_multigraph
Oct 5, 2022
Merged

Add is_multigraph to PG and change has_duplicate_edges to use types#2708
rapids-bot[bot] merged 13 commits intorapidsai:branch-22.10from
eriknw:pg_is_multigraph

Conversation

@eriknw
Copy link
Contributor

@eriknw eriknw commented Sep 20, 2022

Closes #2591

Also, change default graph type to MultiGraph.
allow_multi_edges keyword was renamed to do_expensive_check, which only occurs when the output graph is not a MultiGraph. Should do_expensive_check default to True or False?

Also, change default greph type to MultiGraph.
`allow_multi_edges` keyword was renamed to `do_expensive_check`, which
only occurs when the output graph is not a MultiGraph.
Should `do_expensive_check` default to True or False?
@eriknw eriknw requested a review from a team as a code owner September 20, 2022 03:26
@codecov-commenter
Copy link

codecov-commenter commented Sep 20, 2022

Codecov Report

Base: 59.75% // Head: 59.74% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (ec90585) compared to base (f95ee18).
Patch coverage: 53.57% of modified lines in pull request are covered.

Additional details and impacted files
@@               Coverage Diff                @@
##           branch-22.10    #2708      +/-   ##
================================================
- Coverage         59.75%   59.74%   -0.02%     
================================================
  Files               111      111              
  Lines              6418     6436      +18     
================================================
+ Hits               3835     3845      +10     
- Misses             2583     2591       +8     
Impacted Files Coverage Δ
python/cugraph/cugraph/gnn/graph_store.py 68.83% <0.00%> (ø)
...h/cugraph/gnn/pyg_extensions/data/cugraph_store.py 81.49% <ø> (ø)
...ugraph/cugraph/dask/structure/mg_property_graph.py 13.36% <41.66%> (+0.59%) ⬆️
python/cugraph/cugraph/structure/property_graph.py 95.84% <66.66%> (-0.94%) ⬇️
python/cugraph/cugraph/structure/graph_classes.py 80.00% <0.00%> (+1.02%) ⬆️

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.

@alexbarghi-nv alexbarghi-nv added breaking Breaking change improvement Improvement / enhancement to an existing function labels Sep 21, 2022
@alexbarghi-nv alexbarghi-nv added this to the 22.10 milestone Sep 21, 2022
@eriknw
Copy link
Contributor Author

eriknw commented Sep 22, 2022

Renamed do_expensive_check to check_multi_edges.

I just noticed allow_multi_edges is still used in cugraph_service. Should I take a look at this and try to figure out what to do?

Copy link
Member

@VibhuJawa VibhuJawa left a comment

Choose a reason for hiding this comment

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

LGTM

@eriknw
Copy link
Contributor Author

eriknw commented Oct 1, 2022

@alexbarghi-nv can you comment on the test failure here?

It is failing on cugraph/tests/test_pyg_extensions.py:test_get_subgraph

The edge data from the PropertyGraph is:

   _EDGE_ID_  _SRC_  _DST_    _TYPE_
0          0      3      1       cow
1          1      3      2       cow
2          2      1      4      duck
3          3      2      3      duck
4          4      2      4      duck
5          5      0      1     horse
6          6      0      2     horse
7          7      2      3  mongoose
8          8      1      4  mongoose
9          9      4      3     snake

and

   _EDGE_ID_  _SRC_  _DST_ _TYPE_
0          0      1      4    cat
1          1      2      3    cat
2          2      2      4    cat
3          3      0      2    dog
4          4      3      2    dog
5          5      4      3    dog
6          6      0      1    pig
7          7      2      3    pig
8          8      3      1    pig
9          9      1      4    pig

The test is dropping duplicates based only on SRC and DST columns, but not on TYPE. I don't know what the test is supposed to be testing; what is correct?

@BradReesWork
Copy link
Member

rerun tests

1 similar comment
@BradReesWork
Copy link
Member

rerun tests

@eriknw
Copy link
Contributor Author

eriknw commented Oct 3, 2022

pyg tests updated based on feedback from @alexbarghi-nv 🤞

Copy link
Contributor

@rlratzel rlratzel 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, just a minor suggestion about a docstring.

@eriknw
Copy link
Contributor Author

eriknw commented Oct 4, 2022

@alexbarghi-nv I fixed test_pyg_extensions.py:test_get_subgraph, but now test_pyg_extensions.py:test_neighbor_sample_multi_vertex is failing.

In this test, for cugraph_edge_type, the data from pG.get_edge_data is:

   _EDGE_ID_  _SRC_  _DST_ _TYPE_ edge_type
0          0      3      1    cow       cow
1          1      3      2    cow       cow

but data from row_dict for "cow" is

"black__cow__brown": [0]

@alexbarghi-nv
Copy link
Member

This issue didn't show up for me. Investigating...

@eriknw eriknw requested a review from a team as a code owner October 4, 2022 21:40
Copy link
Contributor

@acostadon acostadon left a comment

Choose a reason for hiding this comment

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

I didn't find any issues. Looks good.

@eriknw
Copy link
Contributor Author

eriknw commented Oct 4, 2022

Thanks all for the reviews and help. Now we're waiting on CI...

@eriknw
Copy link
Contributor Author

eriknw commented Oct 5, 2022

The fix to sampling_utils_impl.cuh in this PR appears to also fix #2770

@rlratzel
Copy link
Contributor

rlratzel commented Oct 5, 2022

@gpucibot merge

@VibhuJawa
Copy link
Member

@gpucibot merge

@rapids-bot rapids-bot bot merged commit eb1d740 into rapidsai:branch-22.10 Oct 5, 2022
nv-rliu pushed a commit to nv-rliu/cugraph-gnn that referenced this pull request Jul 15, 2025
…es (#2708)

Closes #2591 

Also, change default graph type to MultiGraph.
`allow_multi_edges` keyword was renamed to `do_expensive_check`, which only occurs when the output graph is not a MultiGraph. Should `do_expensive_check` default to True or False?

Authors:
  - Erik Welch (https://github.com/eriknw)

Approvers:
  - Vibhu Jawa (https://github.com/VibhuJawa)
  - Don Acosta (https://github.com/acostadon)
  - Chuck Hastings (https://github.com/ChuckHastings)
  - Rick Ratzel (https://github.com/rlratzel)

URL: rapidsai/cugraph#2708
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Breaking change improvement Improvement / enhancement to an existing function

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] PG has_duplicates does not check for type

8 participants