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

forces Raw move to CPU #1107

Merged
merged 6 commits into from
Aug 29, 2023
Merged

forces Raw move to CPU #1107

merged 6 commits into from
Aug 29, 2023

Conversation

Intron7
Copy link
Member

@Intron7 Intron7 commented Aug 28, 2023

Hey,
so I found that the implementation I used to force Raw to the CPU was not covering all cases. I added a second check. I should now always move .X to the CPU.

@Intron7 Intron7 added this to the 0.10.0 milestone Aug 28, 2023
@Intron7 Intron7 requested a review from ivirshup August 28, 2023 14:10
Copy link
Member

@ivirshup ivirshup left a comment

Choose a reason for hiding this comment

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

Thanks!

Is there a test case that needs updating to include whatever was failing before?

@codecov
Copy link

codecov bot commented Aug 28, 2023

Codecov Report

Merging #1107 (870e326) into main (70c5d73) will decrease coverage by 32.31%.
Report is 3 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1107       +/-   ##
===========================================
- Coverage   84.76%   52.46%   -32.31%     
===========================================
  Files          36       36               
  Lines        5128     5133        +5     
===========================================
- Hits         4347     2693     -1654     
- Misses        781     2440     +1659     
Flag Coverage Δ
gpu-tests 52.46% <100.00%> (+0.08%) ⬆️

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

Files Changed Coverage Δ
anndata/_core/raw.py 67.62% <100.00%> (-11.94%) ⬇️

... and 23 files with indirect coverage changes

@Intron7
Copy link
Member Author

Intron7 commented Aug 29, 2023

@ivirshup I added two tests. I hope I put them into the right place.

@ivirshup ivirshup self-requested a review August 29, 2023 11:35
Copy link
Member

@ivirshup ivirshup left a comment

Choose a reason for hiding this comment

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

Yep, I made some minor changes

  • I think it's a little confusing to have names from both cupyx.scipy.sparse and scipy.sparse imported, so I've qualified their usage
  • I think isspmatrix_csr will be deprecated, so I've replaced it with an instance check

@ivirshup ivirshup enabled auto-merge (squash) August 29, 2023 11:36
@ivirshup ivirshup merged commit 1eaa56c into main Aug 29, 2023
12 checks passed
@ivirshup ivirshup deleted the raw_gpu_fix branch August 29, 2023 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants