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

test_ecg_clustering is too sensitive to different RAPIDS_DATA_SET_ROOT_DIR formats #3235

Closed
Tracked by #3256
rlratzel opened this issue Feb 6, 2023 · 0 comments · Fixed by #3779
Closed
Tracked by #3256
Assignees
Labels
bug Something isn't working

Comments

@rlratzel
Copy link
Contributor

rlratzel commented Feb 6, 2023

Users should be able to set RAPIDS_DATASET_ROOT_DIR to an absolute path, or a path relative to their CWD, a path with sym links, etc. However, test_ecg_clustering only allows RAPIDS_DATASET_ROOT_DIR to be set in a particular way, likely due to the code here.

As an alternative to just fixing the code in test_ecg.py could be to migrate this test to use the datasets package.

@rlratzel rlratzel added the Fix label Feb 6, 2023
@rlratzel rlratzel self-assigned this Feb 6, 2023
@kingmesal kingmesal added bug Something isn't working and removed Fix labels Feb 9, 2023
@rlratzel rlratzel assigned nv-rliu and unassigned rlratzel Aug 7, 2023
rapids-bot bot pushed a commit that referenced this issue Aug 16, 2023
Closes #3235 

This PR cleans up the `test_ecg.py` unit test. It removes the old way of comparing networkx results to cugraph, which used `PurePath` to compare the location of the stored dataset when getting networkx results to compare with cugraph.

Since the cugraph tests use the `Datasets` API, the golden results can be fetched based on the name of the dataset being used.

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

Approvers:
  - Alex Barghi (https://github.com/alexbarghi-nv)

URL: #3779
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 a pull request may close this issue.

3 participants