Skip to content

[REVIEW] ENH Update Force Atlas 2 notebook, wrapper and coding style - #1273

Merged
BradReesWork merged 17 commits into
rapidsai:branch-0.17from
hlinsen:fa2-notebook
Nov 20, 2020
Merged

[REVIEW] ENH Update Force Atlas 2 notebook, wrapper and coding style#1273
BradReesWork merged 17 commits into
rapidsai:branch-0.17from
hlinsen:fa2-notebook

Conversation

@hlinsen

@hlinsen hlinsen commented Nov 17, 2020

Copy link
Copy Markdown
Contributor

This PR updates the notebook for the blog post, the Python bindings and addresses the FIXMEs related to stream usage.

@hlinsen
hlinsen requested review from a team as code owners November 17, 2020 09:04
@review-notebook-app

Copy link
Copy Markdown

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@GPUtester

Copy link
Copy Markdown
Contributor

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

@codecov-io

codecov-io commented Nov 17, 2020

Copy link
Copy Markdown

Codecov Report

Merging #1273 (7c663ee) into branch-0.17 (6461951) will increase coverage by 1.20%.
The diff coverage is 76.89%.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.17    #1273      +/-   ##
===============================================
+ Coverage        56.59%   57.80%   +1.20%     
===============================================
  Files               62       63       +1     
  Lines             2564     2782     +218     
===============================================
+ Hits              1451     1608     +157     
- Misses            1113     1174      +61     
Impacted Files Coverage Δ
python/cugraph/__init__.py 100.00% <ø> (ø)
python/cugraph/comms/comms.py 34.52% <0.00%> (-0.85%) ⬇️
python/cugraph/structure/symmetrize.py 70.73% <ø> (ø)
python/cugraph/layout/force_atlas2.py 52.94% <33.33%> (-7.06%) ⬇️
python/cugraph/dask/centrality/katz_centrality.py 34.78% <34.78%> (ø)
python/cugraph/utilities/nx_factory.py 81.35% <41.66%> (-10.32%) ⬇️
python/cugraph/traversal/sssp.py 78.72% <77.77%> (-7.49%) ⬇️
python/cugraph/utilities/utils.py 72.61% <78.78%> (+3.38%) ⬆️
python/cugraph/traversal/bfs.py 82.97% <79.31%> (-9.33%) ⬇️
python/cugraph/structure/convert_matrix.py 83.01% <81.81%> (-5.88%) ⬇️
... and 7 more

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 1be056b...7c663ee. Read the comment docs.

@BradReesWork BradReesWork added 3 - Ready for Review improvement Improvement / enhancement to an existing function labels Nov 17, 2020
@BradReesWork BradReesWork added this to the 0.17 milestone Nov 17, 2020

@ChuckHastings ChuckHastings left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

lgtm

@afender afender left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Consider removing your local output for the first notebook cell :

Requirement already satisfied: fa2 in /home/hlinsenmaier/anaconda3/envs/cugraph_dev2/lib/python3.8/site-packages (0.3.5)
 
Requirement already satisfied: scipy in /home/hlinsenmaier/anaconda3/envs/cugraph_dev2/lib/python3.8/site-packages (from fa2) (1.5.3)
 
Requirement already satisfied: numpy in /home/hlinsenmaier/anaconda3/envs/cugraph_dev2/lib/python3.8/site-packages (from fa2) (1.19.4)
 
Requirement already satisfied: tqdm in /home/hlinsenmaier/anaconda3/envs/cugraph_dev2/lib/python3.8/site-packages (from fa2) (4.52.0)

@hlinsen

hlinsen commented Nov 18, 2020

Copy link
Copy Markdown
Contributor Author

Consider removing your local output for the first notebook cell :

Requirement already satisfied: fa2 in /home/hlinsenmaier/anaconda3/envs/cugraph_dev2/lib/python3.8/site-packages (0.3.5)
 
Requirement already satisfied: scipy in /home/hlinsenmaier/anaconda3/envs/cugraph_dev2/lib/python3.8/site-packages (from fa2) (1.5.3)
 
Requirement already satisfied: numpy in /home/hlinsenmaier/anaconda3/envs/cugraph_dev2/lib/python3.8/site-packages (from fa2) (1.19.4)
 
Requirement already satisfied: tqdm in /home/hlinsenmaier/anaconda3/envs/cugraph_dev2/lib/python3.8/site-packages (from fa2) (4.52.0)

Fixed in commit cc7124c

@afender afender left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm!

@ajschmidt8 ajschmidt8 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good. We'll need the new Notebook dependencies added to our rapids-notebook-env metapackage below. Please open a PR for that before this can be merged. Also, if these new dependencies change frequently, please consider pinning them as well (though I'll leave that to your discretion since I don't know much about them).

https://github.com/rapidsai/integration/blob/branch-0.17/conda/recipes/rapids-notebook-env/meta.yaml

@ajschmidt8 ajschmidt8 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

PR rapidsai/integration#174 is merged, so this is good to go. thanks Hugo

@BradReesWork
BradReesWork merged commit 66f916e into rapidsai:branch-0.17 Nov 20, 2020
@hlinsen
hlinsen deleted the fa2-notebook branch April 15, 2021 12:11
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants