-
Notifications
You must be signed in to change notification settings - Fork 304
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
Refactor Leiden #3327
Refactor Leiden #3327
Conversation
…iden, need to add more to make it complete
…ce_e_by_dst_key to support reduce op on tuple data types
…er during development
…e and construct decision graph
…to leiden-on-22-12
… leiden-aggregated graph
…s, fixes mis implementation
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.
This PR does not update C++ files, approving as a C++ reviewer.
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 overall, thanks. I just had a few comments.
|
||
# FIXME: the networkx.Graph type used in the type annotation for | ||
# induced_subgraph() is specified using a string literal to avoid depending on |
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.
This comment needs to be updated since it still mentions induce_subgraph()
. Maybe just rewrite it so it doesn't mention an API at all if it's going to be copy-pasted elsewhere.
# Temporarily suppress warnings till networkX fixes deprecation warnings | ||
# (Using or importing the ABCs from 'collections' instead of from | ||
# 'collections.abc' is deprecated, and in 3.8 it will stop working) for |
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 we should start removing this Nx warning supression since we should know about deprecations until Nx is completely removed from our tests.
# Test data | ||
# ============================================================================= | ||
|
||
_test_data = { |
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'm assuming black did this, but can the test data be reformatted to have >1 number per line? This is taking up hundreds of lines with no improvement to readability. You should be able to do this:
Black reformats entire files in place. It doesn’t reformat lines that end with # fmt: skip or blocks that start with # fmt: off and end with # fmt: on.
/merge |
A CAPI implementation of Leiden is available and this PR:
Implement PLC Leiden by leveraging the CAPI.
Refactor the python SG implementation of Leiden by leveraging the PLC implementation.
Add a python MG implementation of Leiden with tests.
closes #2487
closes #2489
closes #2490
closes #2491