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.

Sign up for GitHub

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

Jump to bottom

Use MNMG version of ECG in python layer instead, and remove legacy ECG and Louvain #4514

Merged
merged 22 commits into from
Jul 8, 2024
Merged

Use MNMG version of ECG in python layer instead, and remove legacy ECG and Louvain #4514

merged 22 commits into from
Jul 8, 2024

Conversation

naimnv
Copy link
Contributor

@naimnv naimnv commented Jul 1, 2024 •
edited
Loading

This PR

  • removes legacy ECG and Louvain code
  • updates c_api, plc and python code to use MNMG ECG instead of legacy version

Closes #2494

@naimnv naimnv requested review from a team as code owners July 1, 2024 23:22
@copy-pr-bot copy-pr-bot
Copy link

copy-pr-bot bot commented Jul 1, 2024

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@naimnv naimnv added feature request New feature or request non-breaking Non-breaking change labels Jul 1, 2024
@naimnv naimnv added this to the 24.08 milestone Jul 1, 2024
@naimnv
Copy link
Contributor Author

naimnv commented Jul 1, 2024

/okay to test

@naimnv
Copy link
Contributor Author

naimnv commented Jul 1, 2024

/okay to test

@naimnv naimnv requested review from rlratzel and ChuckHastings July 1, 2024 23:50
@naimnv naimnv changed the title Use MNMG version of ECG in python layer instead of legacy version Use MNMG version of ECG in python layer instead, and remove legacy ECG and Louvain Jul 1, 2024
@naimnv
Copy link
Contributor Author

naimnv commented Jul 2, 2024

/okay to test

@naimnv naimnv self-assigned this Jul 2, 2024
This was linked to issues Jul 2, 2024
MNMG ECG - Remove Legacy C/C++ #3576
Closed
MNMG ECG - Define Python API #3577
Closed
MNMG ECG - Define Python Unit Tests #3578
Closed
MNMG ECG - Python Implementation #3579
Closed
MNMG ECG - Remove legacy python #3580
Closed
@naimnv naimnv requested a review from a team as a code owner July 2, 2024 11:20
@naimnv
Copy link
Contributor Author

naimnv commented Jul 2, 2024

/okay to test

ChuckHastings
ChuckHastings approved these changes Jul 2, 2024
View reviewed changes
Copy link
Collaborator

@ChuckHastings ChuckHastings left a comment

Choose a reason for hiding this comment

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

LGTM. Glad to be done with a legacy implementation.

rlratzel
rlratzel requested changes Jul 2, 2024
View reviewed changes
Copy link
Contributor

@rlratzel rlratzel left a comment

Choose a reason for hiding this comment

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

LGTM but we're deprecating support for NetworkX input types (we're going to start telling users to use nx-cugraph if they want to use NetworkX types) so I made some suggestions.

python/cugraph/cugraph/community/ecg.py Outdated Show resolved Hide resolved
@naimnv
Copy link
Contributor Author

naimnv commented Jul 2, 2024

/okay to test

@naimnv naimnv requested review from rlratzel and KyleFromNVIDIA July 2, 2024 21:38
@naimnv
Copy link
Contributor Author

naimnv commented Jul 3, 2024

/okay to test

@naimnv
Copy link
Contributor Author

naimnv commented Jul 3, 2024

/okay to test

@naimnv
Copy link
Contributor Author

naimnv commented Jul 3, 2024

/okay to test

@naimnv
Copy link
Contributor Author

naimnv commented Jul 3, 2024

/okay to test

@naimnv
Copy link
Contributor Author

naimnv commented Jul 6, 2024

/okay to test

@ChuckHastings ChuckHastings added breaking Breaking change and removed non-breaking Non-breaking change labels Jul 8, 2024
@ChuckHastings
Copy link
Collaborator

Labeling this a breaking change. This does delete the legacy implementation of Louvain... so it breaks our C++ API by deleting an obsolete feature.

rlratzel
rlratzel approved these changes Jul 8, 2024
View reviewed changes
python/cugraph/cugraph/community/ecg.py
Comment on lines +111 to +116
if isNx:
warning_msg = (
" We are deprecating support for handling "
"NetworkX types in the next release."
)
warnings.warn(warning_msg, UserWarning)
Copy link
Contributor

Choose a reason for hiding this comment

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

This message doesn't hurt but isn't needed since ensure_cugraph_obj_for_nx() will handle the deprecation warning in this PR: https://github.com/rapidsai/cugraph/pull/4493/files#diff-62286c02304119d912dbd0491893a58c1dea11bd0addcc80fadb3d129d21029f

@ChuckHastings
Copy link
Collaborator

/merge

KyleFromNVIDIA
KyleFromNVIDIA approved these changes Jul 8, 2024
View reviewed changes
Copy link
Contributor

@KyleFromNVIDIA KyleFromNVIDIA left a comment

Choose a reason for hiding this comment

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

Approved CMake and Python build changes

@rapids-bot rapids-bot bot merged commit 127d3be into rapidsai:branch-24.08 Jul 8, 2024
135 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KyleFromNVIDIA KyleFromNVIDIA KyleFromNVIDIA approved these changes

@ChuckHastings ChuckHastings ChuckHastings approved these changes

@rlratzel rlratzel rlratzel approved these changes

Assignees

@naimnv naimnv

Labels
breaking Breaking change CMake cuGraph feature request New feature or request python
Projects
None yet
Milestone
24.08
4 participants
@naimnv @ChuckHastings @rlratzel @KyleFromNVIDIA