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

Make cluster gcn reproducible #937

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Conversation

kjun9
Copy link
Contributor

@kjun9 kjun9 commented Feb 25, 2020

This updates cluster gcn's node generator to use the random_state utility that's now available, and adds some tests to make sure the model is reproducible when we set the global stellargraph seed.

Part of #749

"""

def __init__(self, G, clusters=1, q=1, lam=0.1, name=None):
def __init__(self, G, clusters=1, q=1, lam=0.1, name=None, seed=None):
Copy link

Choose a reason for hiding this comment

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

Refactor this function to reduce its Cognitive Complexity from 17 to the 15 allowed.

@codeclimate
Copy link

codeclimate bot commented Feb 25, 2020

Code Climate has analyzed commit d078921 and detected 0 issues on this pull request.

View more on Code Climate.

@stellar-graph-bot
Copy link

Codecov Report

Merging #937 into develop will increase coverage by 0.4%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           develop    #937     +/-   ##
=========================================
+ Coverage     83.9%   84.3%   +0.4%     
=========================================
  Files           53      53             
  Lines         5101    5103      +2     
=========================================
+ Hits          4281    4301     +20     
+ Misses         820     802     -18     
Impacted Files Coverage Δ
stellargraph/mapper/mini_batch_node_generators.py 97.1% <0.0%> (+13.3%) ⬆️

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 76df745...30686c1. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Feb 25, 2020

Codecov Report

Merging #937 into develop will increase coverage by 0.5%.
The diff coverage is 85.9%.

Impacted file tree graph

@@            Coverage Diff            @@
##           develop    #937     +/-   ##
=========================================
+ Coverage     84.3%   84.8%   +0.5%     
=========================================
  Files           53      58      +5     
  Lines         5103    4958    -145     
=========================================
- Hits          4301    4206     -95     
+ Misses         802     752     -50
Impacted Files Coverage Δ
stellargraph/layer/graphsage.py 79.4% <ø> (ø) ⬆️
stellargraph/utils/ensemble.py 0% <0%> (-85.5%) ⬇️
stellargraph/utils/loss.py 0% <0%> (ø) ⬆️
stellargraph/datasets/datasets.py 70.8% <100%> (+20.8%) ⬆️
stellargraph/core/graph.py 98.6% <100%> (ø) ⬆️
stellargraph/mapper/sequences.py 92.1% <100%> (ø) ⬆️
...rgraph/utils/saliency_maps/integrated_gradients.py 100% <100%> (+4.3%) ⬆️
...argraph/interpretability/saliency_maps/__init__.py 100% <100%> (ø)
...ph/utils/saliency_maps/integrated_gradients_gat.py 100% <100%> (+10.3%) ⬆️
stellargraph/version.py 100% <100%> (ø) ⬆️
... and 19 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 30686c1...d078921. Read the comment docs.

@kjun9 kjun9 marked this pull request as ready for review February 27, 2020 23:41
targets = np.random.rand(petersen_graph.number_of_nodes(), target_size)
assert_reproducible(
lambda: cluster_gcn_nai(petersen_graph, targets, 4, 2, shuffle=shuffle)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel that we need a test to also make sure that if the seed is different, then the models are indeed different. Otherwise, we have not established a causal link between the random seed value and initial model weights.

Copy link
Contributor

@PantelisElinas PantelisElinas left a comment

Choose a reason for hiding this comment

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

Hi @kjun9

You might want to have a look at my suggestion for another test, otherwise this is good to merge.

P.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants