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

Fix: Dense OOC Concat With Aligned Elements Memory Issue #1169

Merged
merged 3 commits into from
Oct 10, 2023

Conversation

selmanozleyen
Copy link
Member

Please see scverse/anndata-tutorials#18 (comment). This PR should fix this memory issue.

Ping: @ivirshup @flying-sheep

@codecov
Copy link

codecov bot commented Oct 6, 2023

Codecov Report

Merging #1169 (979a894) into main (6a969eb) will decrease coverage by 2.19%.
Report is 2 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1169      +/-   ##
==========================================
- Coverage   84.97%   82.79%   -2.19%     
==========================================
  Files          36       36              
  Lines        5364     5364              
==========================================
- Hits         4558     4441     -117     
- Misses        806      923     +117     
Flag Coverage Δ
gpu-tests ?

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

Files Coverage Δ
anndata/experimental/merge.py 91.28% <100.00%> (ø)

... and 7 files with indirect coverage changes

@ivirshup
Copy link
Member

ivirshup commented Oct 8, 2023

@selmanozleyen, could you make a quick demonstration of the performance benefits from this?

@selmanozleyen
Copy link
Member Author

Hey, I modified the fix as it seemed like it would expand to more cases (e.g. when they are all not the same). The changes on results can be seen from the tutorial notebook scverse/anndata-tutorials@b12e539#r1349746130

@ivirshup
Copy link
Member

ivirshup commented Oct 9, 2023

From previous discussion about this, my impression was that da.from_array(a, chunks="auto") would use a.chunks as the chunks if it was defined.

I will try and take a look, but it looks to me like these changes could be coming from the use of write_chunked.

@selmanozleyen
Copy link
Member Author

selmanozleyen commented Oct 9, 2023

my impression was that da.from_array(a, chunks="auto") would use a.chunks as the chunks if it was defined.

I will try and take a look, but it looks to me like these changes could be coming from the use of write_chunked.

I see, I initially tried that but it didn't work for some reason. If I switch to main branch the results would be the same as the bad one.

@flying-sheep
Copy link
Member

That tracks with the documentation:

The word “auto” which acts like the above, but uses a configuration value array.chunk-size for the chunk size

No word of reusing the array chunks.

@flying-sheep flying-sheep added this to the 0.10.2 milestone Oct 10, 2023
@flying-sheep flying-sheep merged commit 08ef471 into scverse:main Oct 10, 2023
13 of 14 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/anndata that referenced this pull request Oct 10, 2023
flying-sheep pushed a commit that referenced this pull request Oct 10, 2023
…d Elements Memory Issue) (#1179)

Co-authored-by: Selman Özleyen <[email protected]>
@ivirshup
Copy link
Member

@flying-sheep, shouldn't this get a release note and a test?

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.

3 participants