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

Refactor MG Centrality Tests #4197

Merged
merged 19 commits into from
Mar 14, 2024

Conversation

nv-rliu
Copy link
Contributor

@nv-rliu nv-rliu commented Feb 27, 2024

Addresses #4187

This PR makes improvements to old testing conventions used in cugraph.centrality MG tests

Instead of using nested fixtures. eg. input_expected_output -> input_combo -> fixture_params which can be confusing, the @pytest.mark.parametrize marker is used to iterate through combinations of parameters used for testing. The fixtures are also replaced by functions used to create SG and MG graphs.

@nv-rliu nv-rliu added this to the 24.04 milestone Feb 27, 2024
@nv-rliu nv-rliu added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change and removed python labels Feb 27, 2024
@nv-rliu nv-rliu force-pushed the b2404-cleanup-mg-tests branch from 9f792d4 to 73840cd Compare February 27, 2024 16:58
@nv-rliu nv-rliu marked this pull request as ready for review February 27, 2024 17:53
@nv-rliu nv-rliu requested a review from a team as a code owner February 27, 2024 17:53
@nv-rliu nv-rliu requested a review from jnke2016 February 27, 2024 17:59
@nv-rliu nv-rliu force-pushed the b2404-cleanup-mg-tests branch from 39cb5f6 to b3ec32c Compare March 8, 2024 21:41
@nv-rliu
Copy link
Contributor Author

nv-rliu commented Mar 8, 2024

Some unrelated tests are failing and I'm currently investigating/reproducing this locally.

@nv-rliu nv-rliu force-pushed the b2404-cleanup-mg-tests branch from 7ec59c8 to 0513b81 Compare March 11, 2024 03:01
@nv-rliu nv-rliu force-pushed the b2404-cleanup-mg-tests branch from eaeec44 to 39a4e97 Compare March 13, 2024 18:50
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.

Looks good but I was wondering about some of the dataset.unload() calls and if we need them. Do these tests have problems if the unload() calls aren't there, or were they added just in case?

Copy link
Contributor

@acostadon acostadon left a comment

Choose a reason for hiding this comment

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

Looks Great

Copy link
Contributor

@jnke2016 jnke2016 left a comment

Choose a reason for hiding this comment

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

Looks good to me. And nice work tracking down that PR failure on CI

@rlratzel
Copy link
Contributor

/merge

@rapids-bot rapids-bot bot merged commit fa096f0 into rapidsai:branch-24.04 Mar 14, 2024
137 of 138 checks passed
@nv-rliu nv-rliu deleted the b2404-cleanup-mg-tests branch March 14, 2024 20:58
rapids-bot bot pushed a commit that referenced this pull request May 14, 2024
Addresses #4187 

This PR makes improvements to old MG testing conventions used in these testing directories:
 - centrality
 - comms
 - community
 - components
 - core
 - internals

The goals of this PR is to improve readability and use of the MG tests. Changes are similar to #4197 
> Instead of using nested fixtures. eg. `input_expected_output -> input_combo -> fixture_params` which can be confusing, the `@pytest.mark.parametrize` marker is used to iterate through combinations of parameters used for testing. The fixtures are also replaced by functions used to create SG and MG graphs.

By using the Datasets API (which now supports MG Graphs thanks to @huiyuxie), the number of imports and `testing.utils` functions can be significantly reduced.

Authors:
  - Ralph Liu (https://github.com/nv-rliu)

Approvers:
  - Rick Ratzel (https://github.com/rlratzel)

URL: #4244
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants