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

Create a notebook comparing nx and cuGraph using synthetic data #3135

Merged
merged 18 commits into from
Jan 27, 2023
Merged

Create a notebook comparing nx and cuGraph using synthetic data #3135

merged 18 commits into from
Jan 27, 2023

Conversation

acostadon
Copy link
Contributor

Rapids visualization group asked for some data comparing cuGraph algos to nx.
Used Release notebook as a base, replaced datasets with RMAT data generator.

Closes #3134

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@acostadon acostadon self-assigned this Jan 12, 2023
@acostadon acostadon added non-breaking Non-breaking change doc Documentation feature request New feature or request labels Jan 12, 2023
@acostadon acostadon added this to the 23.02 milestone Jan 12, 2023
@acostadon acostadon added P1 and removed feature request New feature or request labels Jan 12, 2023
@acostadon acostadon marked this pull request as ready for review January 13, 2023 15:00
@acostadon acostadon requested a review from a team as a code owner January 13, 2023 15:00
@BradReesWork
Copy link
Member

minor text changed needed

Copy link
Member

@BradReesWork BradReesWork left a comment

Choose a reason for hiding this comment

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

some text needs to be updated and the generator fixed

@exactlyallan
Copy link
Member

From a content perspective, this notebook works well for the website benchmark. For viz: rapidsai/cuml#5103 (comment)

@codecov-commenter
Copy link

codecov-commenter commented Jan 17, 2023

Codecov Report

Base: 55.22% // Head: 55.22% // No change to project coverage 👍

Coverage data is based on head (956a4bd) compared to base (5c021fb).
Patch has no changes to coverable lines.

Additional details and impacted files
@@              Coverage Diff              @@
##           branch-23.02    #3135   +/-   ##
=============================================
  Coverage         55.22%   55.22%           
=============================================
  Files               148      148           
  Lines              9543     9543           
=============================================
  Hits               5270     5270           
  Misses             4273     4273           

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.

@BradReesWork
Copy link
Member

The notebook need a header block at the top explain what it is doing

@BradReesWork
Copy link
Member

Cell 3: The names of the datasets are confusing. What not "data_scale_11" instead of "data_11th" . I'm not sure what the "11th" is

@BradReesWork
Copy link
Member

Cel 4 - the text is wrong - this is not a data reader

@BradReesWork
Copy link
Member

cell 4:
print('Generating a dataframe of ' + str(len(_gdf)) + '...')
should be past tense
print(f'Generated {str(len(_gdf))} edges' )

@BradReesWork
Copy link
Member

Cell 5 - why not combine the two cuGraph creates into one and have a "direction" flag passed in?

It used to be two functions since there was Graph and DiGraph, but that is no longer the case

@BradReesWork
Copy link
Member

Cell 6 - Katz still have the Nx object function

@BradReesWork
Copy link
Member

Cell 12: PageRank

    _G = create_cu_directed_graph(_df, transpose=True)
    if _G.is_directed():
        _G = _G.to_undirected()

why not just create an undirected graph?

@BradReesWork
Copy link
Member

BradReesWork commented Jan 24, 2023

Cell 3:
What are the Scale number so small? We want to compare against larger graphs, max should be scale 25

Do Scales: (or pick 3 or 4)
10
14
16
19
22
24
25

NOTE: using the large datasets will be very very slow in Nx
24 and 25 will be too large

Copy link
Member

@BradReesWork BradReesWork left a comment

Choose a reason for hiding this comment

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

see comments

@kingmesal kingmesal mentioned this pull request Jan 24, 2023
@BradReesWork
Copy link
Member

/merge

@rapids-bot rapids-bot bot merged commit cd2a91b into rapidsai:branch-23.02 Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Documentation non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA]: Notebook comparing nx to cugraph with synthetic data
4 participants