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

Graph::subgraphFromNodes with neighbors #373

Merged
merged 5 commits into from
Jul 16, 2019

Conversation

manpen
Copy link
Contributor

@manpen manpen commented Jul 6, 2019

This PR adds a new feature to Graph::subgraphFromNodes which originally created an induced subgraph on the set nodes of nodes provided. In some applications it however also makes sense to include their neighbours -- hence the new feature which can be enabled by setting the parameter includeOutNeighbors or includeInNeighbors. They default to false, so the standard behaviour does not change.

All edges in added must have at least one endpoint in nodes, the other one is either in nodes or in the set of neighbors included.

This PR also adds:

  • CPP Unit tests from subgraphFromNodes
  • Python Unit tests for subgraphFromNodes (with an invitation to add more Python test for Graph)
  • Shebang line in all Python tests and exectuable flags, so we can easily execute those tests from the command line (if you do not like it in this PR, I can drop the commit and open another one).

avdgrinten
avdgrinten previously approved these changes Jul 15, 2019
Copy link
Contributor

@avdgrinten avdgrinten left a comment

Choose a reason for hiding this comment

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

LGTM.

As a side note: subgraphFromNodes looks like a function that should really be in GraphTools.

@angriman
Copy link
Member

I just have a minor comment, apart from that the PR looks good to me.

@manpen
Copy link
Contributor Author

manpen commented Jul 15, 2019

Thanks for the review. I added the comments (only change in the force-push).
@avdgrinten: I completely agree, and TBH, I first looked into GraphTools when I was searching this feature. There are additional methods such as merge (?), to(Unw|W)eighted, transpose, BFS that I do not really see as part of Graph. However, at the same time, these are typically methods used by user code and I do not really think it's worth breaking them.

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

3 participants