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

Expose simplicial set functions #4711

Merged
merged 14 commits into from
May 24, 2022
Merged

Expose simplicial set functions #4711

merged 14 commits into from
May 24, 2022

Conversation

viclafargue
Copy link
Contributor

@viclafargue viclafargue commented Apr 26, 2022 •
edited

This PR closes issues #3123, #4704 and #4292

@viclafargue viclafargue requested a review from a team as a code owner April 26, 2022 18:10
@github-actions github-actions bot added the Cython / Python Cython or Python issue label Apr 26, 2022
cjnolet
cjnolet requested changes Apr 28, 2022
View reviewed changes
Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

This is looking really good so far, but I think the original c++ functions might require a few small changes before it can be used directly in Scanpy.

python/cuml/manifold/simpl_set.pyx Show resolved Hide resolved
python/cuml/manifold/simpl_set.pyx Outdated Show resolved Hide resolved
python/cuml/manifold/simpl_set.pyx Outdated Show resolved Hide resolved
python/cuml/manifold/simpl_set.pyx Outdated Show resolved Hide resolved
python/cuml/tests/test_simpl_set.py Outdated Show resolved Hide resolved
python/cuml/tests/test_simpl_set.py Outdated Show resolved Hide resolved
python/cuml/tests/test_simpl_set.py Outdated Show resolved Hide resolved
python/cuml/manifold/simpl_set.pyx Outdated Show resolved Hide resolved
python/cuml/manifold/simpl_set.pyx Outdated Show resolved Hide resolved
@viclafargue viclafargue requested a review from a team as a code owner April 29, 2022 13:10
@divyegala
Copy link
Member

@viclafargue could you use "closes" or "fixes" so that linked issues are automatically closed upon merging?

@cjnolet cjnolet mentioned this pull request May 10, 2022
Closed
cjnolet
cjnolet requested changes May 11, 2022
View reviewed changes
Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

There's a lot of changes here since I last reviewed and i wanted to provide quick initial feedback so I don't keep you waiting. I'm going to look through it a little more closely over the next few days but I doubt I'll find much if at all.

python/cuml/tests/test_simpl_set.py Outdated Show resolved Hide resolved
python/cuml/tests/test_umap.py Outdated Show resolved Hide resolved
cpp/include/cuml/manifold/umap.hpp Show resolved Hide resolved
@cjnolet cjnolet added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels May 12, 2022
@cjnolet cjnolet added this to PR-WIP in v22.06 Release via automation May 12, 2022
@cjnolet
Copy link
Member

cjnolet commented May 16, 2022

rerun tests

2 similar comments
@cjnolet
Copy link
Member

cjnolet commented May 17, 2022

rerun tests

@cjnolet
Copy link
Member

cjnolet commented May 17, 2022

rerun tests

v22.06 Release automation moved this from PR-WIP to PR-Reviewer approved May 17, 2022
@cjnolet
Copy link
Member

cjnolet commented May 17, 2022

@gpucibot merge

@cjnolet
Copy link
Member

cjnolet commented May 18, 2022

rerun tests

1 similar comment
@cjnolet
Copy link
Member

cjnolet commented May 18, 2022

rerun tests

v22.06 Release automation moved this from PR-Reviewer approved to PR-Needs review May 18, 2022
@github-actions github-actions bot added the gpuCI gpuCI issue label May 18, 2022
@cjnolet
Copy link
Member

cjnolet commented May 19, 2022

@viclafargue I’ve seen other PRs passing the centos tests without timing out, which is making me think something is unique to this PR which is causing problems. It might help to verify there are no obvious things like memory leaks which could be causing a problem.

@viclafargue viclafargue requested a review from a team as a code owner May 20, 2022 09:12
@cjnolet
Copy link
Member

cjnolet commented May 20, 2022

rerun tests

ajschmidt8
ajschmidt8 approved these changes May 20, 2022
View reviewed changes
Copy link
Member

@ajschmidt8 ajschmidt8 left a comment

Choose a reason for hiding this comment

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

Approving ops-codeowner file changes

v22.06 Release automation moved this from PR-Needs review to PR-Reviewer approved May 20, 2022
@viclafargue
Copy link
Contributor Author

rerun tests

@cjnolet cjnolet removed this from PR-Reviewer approved in v22.06 Release May 23, 2022
@cjnolet cjnolet added this to PR-WIP in v22.08 Release via automation May 23, 2022
@github-actions github-actions bot removed the gpuCI gpuCI issue label May 23, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #4711 (af794cf) into branch-22.06 (15aba09) will increase coverage by 0.02%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##           branch-22.06    #4711      +/-   ##
================================================
+ Coverage         79.89%   79.92%   +0.02%     
================================================
  Files               180      180              
  Lines             11371    11381      +10     
================================================
+ Hits               9085     9096      +11     
+ Misses             2286     2285       -1
Flag Coverage Δ
dask 45.49% <ø> (+0.03%) ⬆️
non-dask 69.55% <ø> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
python/cuml/preprocessing/LabelEncoder.py 93.24% <0.00%> (+2.61%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15aba09...af794cf. Read the comment docs.

@cjnolet
Copy link
Member

cjnolet commented May 23, 2022

@viclafargue I'm honestly at a loss on this one... at this point, you've essentially turned off any code paths that would use the changes in this PR, right? It looks like it's still failing. Hmm...I'm at a loss...

@viclafargue
Copy link
Contributor Author

@viclafargue I'm honestly at a loss on this one... at this point, you've essentially turned off any code paths that would use the changes in this PR, right? It looks like it's still failing. Hmm...I'm at a loss...

Actually, I went back in the commits and only disabled UMAP testing. But, it looks like both simplicial set testing and UMAP testing are causing the issue. I'm looking into it.

@rapids-bot rapids-bot bot merged commit ee0f42e into rapidsai:branch-22.06 May 24, 2022
v22.08 Release automation moved this from PR-WIP to Done May 24, 2022
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this pull request Oct 9, 2023
@viclafargue
Expose simplicial set functions (rapidsai#4711)
dcc0068 
This PR closes issues rapidsai#3123, rapidsai#4704 and rapidsai#4292

Authors:
  - Victor Lafargue (https://github.com/viclafargue)

Approvers:
  - Corey J. Nolet (https://github.com/cjnolet)
  - AJ Schmidt (https://github.com/ajschmidt8)

URL: rapidsai#4711
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lowener lowener lowener left review comments

@cjnolet cjnolet cjnolet approved these changes

@ajschmidt8 ajschmidt8 ajschmidt8 approved these changes

Assignees
No one assigned
Labels
CUDA/C++ Cython / Python Cython or Python issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
No open projects
v22.08 Release
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@viclafargue @divyegala @cjnolet @codecov-commenter @ajschmidt8 @lowener