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

Fixed the bug in LinkSequence that threw an error when no link targets were given #273

Merged
merged 4 commits into from
Oct 16, 2018

Conversation

youph
Copy link
Contributor

@youph youph commented Oct 11, 2018

  • Fixed the bug in LinkSequence class that occurred when no link targets were given to the link iterator.
  • Added a test, test_GraphSAGELinkGenerator_no_targets, covering the case of link generator with no targets (e.g., for prediction)

@youph youph requested a review from adocherty October 11, 2018 07:34
@youph youph self-assigned this Oct 11, 2018
@youph youph added this to the v0.3 Sprint 1 milestone Oct 11, 2018
@youph youph added bug Something isn't working ml labels Oct 11, 2018
@youph youph changed the title Fixed the bug in LinkGenerator when no link targets are given Fixed the bug in LinkGenerator that threw an error when no link targets were given Oct 12, 2018
@youph youph changed the title Fixed the bug in LinkGenerator that threw an error when no link targets were given Fixed the bug in LinkSequence that threw an error when no link targets were given Oct 12, 2018
Copy link
Contributor

@adocherty adocherty left a comment

Choose a reason for hiding this comment

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

The bugfix looks good. I think we can improve the tests a bit then let's merge this.

tests/mapper/test_link_mappers.py Outdated Show resolved Hide resolved
tests/mapper/test_link_mappers.py Outdated Show resolved Hide resolved
@adocherty
Copy link
Contributor

Hi Yuri, I think we should also check the HinSAGE link generator in the same way and I've added such a test. I also looked at the node_generators and I was not testing them with targets well. I've added some tests for those as well to this PR.

@youph
Copy link
Contributor Author

youph commented Oct 16, 2018

@adocherty thanks for adding the tests!

@youph
Copy link
Contributor Author

youph commented Oct 16, 2018

@adocherty strange: I've marked your comments as resolved, but github still says I need to address the change request. Does it wait for you to mark the comments as resolved as well?

@youph youph merged commit 9c18568 into develop Oct 16, 2018
@youph youph deleted the bugfix/link_mappers_without_targets branch October 16, 2018 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants