-
Notifications
You must be signed in to change notification settings - Fork 425
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 HinSAGE reproducible #926
base: develop
Are you sure you want to change the base?
Conversation
@@ -75,7 +75,7 @@ def __init__(self, G, batch_size, schema=None): | |||
def sample_features(self, head_links, batch_num): | |||
pass | |||
|
|||
def flow(self, link_ids, targets=None, shuffle=False): | |||
def flow(self, link_ids, targets=None, shuffle=False, seed=None): |
There was a problem hiding this comment.
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.
Code Climate has analyzed commit 69270e9 and detected 0 issues on this pull request. View more on Code Climate. |
Hmm.. something is still flaky, investigating the test failure now |
Codecov Report
@@ Coverage Diff @@
## develop #926 +/- ##
=========================================
- Coverage 85.4% 84.5% -0.9%
=========================================
Files 66 58 -8
Lines 6774 4956 -1818
=========================================
- Hits 5784 4186 -1598
+ Misses 990 770 -220
Continue to review full report at Codecov.
|
There was a problem hiding this 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 few minor comments on the tests
tests/mapper/test_node_mappers.py
Outdated
] | ||
features, labels = zip(*batches) | ||
features, labels = np.concatenate(features), np.concatenate(labels) | ||
assert all(features == labels) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be:
assert all(features == labels) | |
assert np.array_equal(features, labels) |
tests/mapper/test_node_mappers.py
Outdated
|
||
for i in range(max_iter): | ||
f1, f2 = consecutive_epochs(seq) | ||
comparison_results.add(all(f1 == f2)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here:
comparison_results.add(all(f1 == f2)) | |
comparison_results.add(np.array_equal(f1, f2)) |
tests/mapper/test_node_mappers.py
Outdated
|
||
for i in range(max_iter): | ||
f1, f2 = consecutive_epochs(seq) | ||
comparison_results.add(all(f1 == f2)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could the assertions happen in this line?
comparison_results.add(all(f1 == f2)) | |
if shuffle: | |
assert not np.array_equal(f1, f2) | |
else: | |
assert np.array_equal(f1, f2) |
a54510b
to
69270e9
Compare
It should be blamed on random state use multiple times, which will generate different results. The following steps guarantee consistent results even at every epoch.
|
This is for both NAI and link prediction, which are the two tasks currently available for HinSAGE
Part of #749