-
Notifications
You must be signed in to change notification settings - Fork 154
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
Add cupy support + CI #1066
Add cupy support + CI #1066
Conversation
Signed-off-by: zethson <[email protected]>
for more information, see https://pre-commit.ci
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1066 +/- ##
==========================================
- Coverage 84.28% 82.57% -1.72%
==========================================
Files 35 35
Lines 4932 5112 +180
==========================================
+ Hits 4157 4221 +64
- Misses 775 891 +116
|
Signed-off-by: zethson <[email protected]>
Signed-off-by: zethson <[email protected]>
Signed-off-by: zethson <[email protected]>
Signed-off-by: zethson <[email protected]>
Co-authored-by: Isaac Virshup <[email protected]>
Signed-off-by: zethson <[email protected]>
Signed-off-by: zethson <[email protected]>
Signed-off-by: zethson <[email protected]>
Signed-off-by: zethson <[email protected]>
Signed-off-by: zethson <[email protected]>
So, it runs. But I see some issues:
|
That’s mamba-org/mamba#2059
Why is it bad? Both are dependency resolvers. The ideal way to use package managers would be to pick a single one, and let it install everything in a single step. Since we don’t do that (and probably can’t?), what pip does is correct. Note that it avoids upgrading packages when
The fact that pip updates numpy therefore means that something requires a minimum numpy version greater than the one that micromamba installed. |
Because pip can't actually uninstall conda packages. |
I tested the preprocessing from rapids-singlecell with cupy anndata. There are no issues expect the way views interact with .X replacements. This happens when i want to put a dense matrix in after |
So I have some more small Ideas that I think would be good. I can also start implementing some of them:
|
I think this is getting pretty close to mergable, so I think I'd leave extra features on the actual cupy support out for now. I've opened #1080 to discuss follow up PRs. |
@Intron7, you had mentioned some rules for controlling when this CI was run. Do you think you could link out to this/ maybe help set this up? |
So far I only know that cuML uses that solution. I didnt check out how this works. But I will investigate this. |
If you try to set a dense array for a sparse matrix in a view cuda this happens. Can we handle this a bit more gracefully? Right now I have to copy before the function or use
|
Could you share some code that throws this? |
This works though
|
In this instance I think you can do: rand = sparse.random(100000, 20000, density=0.05,dtype=np.float32, format="csr")
adata = AnnData(X= cpsparse.csr_matrix(rand))
adata = adata[:,:5000]
X = cp.random.rand(100000,5000, dtype= cp.float32)
del adata.X
adata.X = X But yeah the behavior is weird, but not a bug. I'm not really sure what a more graceful way to handle this would be here. I think I could be up for a inplace conversion to actual though. Can you open an issue for this? |
For very small matrices it works but is super slow. Open an issue for the feature #1082 |
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.
Looking good! Once the TODOs are done, I think we’re good to go!
@ivirshup says:
TODO:
- [ ] Consider how much can be done with array_api