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

[c++] Reindexer overrides and fast COO/CSR #1728

Merged
merged 22 commits into from
Jan 19, 2024
Merged

Conversation

beroy
Copy link
Collaborator

@beroy beroy commented Sep 27, 2023

Issue and/or context:

#1611
#1614
#1590
#718
#1610

Changes:

  • libtiledbsoma
  • python apis
  • AxisQuery and some other python code

Notes for Reviewer:

Implement a re-indexer similar to Pandas reindexer
Provide pybind11 for the re-indexer (currently only numpy and soon Arrow interface) for python

  • khash.h and khashl.h are klib library

Expose AxisQuery and fast_csr to use the re-indexer to speedup COO to CSR conversion

For the time being switching between the two indexer (IntIndexer and pandas indexer) is possible using two variables (named use_int_indexer) in _query and fast_csr. As a part of this PR review, I'd like to figure out the best way to have this new feature easily enabled or disabled.

@codecov-commenter
Copy link

codecov-commenter commented Sep 27, 2023

Codecov Report

Merging #1728 (c8e1b51) into main (bc2b823) will increase coverage by 0.06%.
The diff coverage is 87.75%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1728      +/-   ##
==========================================
+ Coverage   75.79%   75.85%   +0.06%     
==========================================
  Files         126      135       +9     
  Lines       10083    10407     +324     
  Branches      161      200      +39     
==========================================
+ Hits         7642     7894     +252     
- Misses       2366     2420      +54     
- Partials       75       93      +18     
Flag Coverage Δ
libtiledbsoma 69.58% <86.61%> (-0.29%) ⬇️
python 89.75% <95.00%> (+0.68%) ⬆️
r 67.72% <ø> (ø)

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

Components Coverage Δ
python_api 89.75% <95.00%> (+0.68%) ⬆️
libtiledbsoma 53.60% <ø> (+0.08%) ⬆️

@johnkerl johnkerl changed the title Cpp reindexer overwrites [c++] Reindexer overrides Sep 27, 2023
@beroy beroy force-pushed the cpp_reindexer_overwrites branch 10 times, most recently from 439f82b to 854c3b8 Compare October 5, 2023 16:34
@beroy beroy force-pushed the cpp_reindexer_overwrites branch 6 times, most recently from e35c92e to a6cd2aa Compare October 11, 2023 20:13
@beroy beroy requested a review from johnkerl October 12, 2023 17:26
@beroy beroy changed the title [c++] Reindexer overrides [c++] Reindexer overrides and Fast COO/CSR Oct 12, 2023
@thetorpedodog
Copy link
Contributor

Reading this now, I have a good idea of how we can add an abstraction in somacore to make this change much less invasive and more straightforward. I have some more work to do here, but this is the start: single-cell-data/SOMA@main...build-index-abstraction

@beroy beroy force-pushed the cpp_reindexer_overwrites branch 2 times, most recently from 06d16b6 to 5f8442f Compare October 12, 2023 23:36
Copy link
Member

@nguyenv nguyenv left a comment

Choose a reason for hiding this comment

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

Thanks for adding this and making the changes. I have one last minor change request, but I think it's good to go.

*
* @section DESCRIPTION
*
* This file manages unit tests for column buffers
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* This file manages unit tests for column buffers
* This file manages unit tests for the reindexer

@beroy beroy merged commit 08931d3 into main Jan 19, 2024
15 checks passed
@beroy beroy deleted the cpp_reindexer_overwrites branch January 19, 2024 16:47
nguyenv pushed a commit that referenced this pull request Jan 19, 2024
somacore v1.0.7 is fully compatible with v1.0.6 and does not introduce
any behavior changes. It does *prepare* us for the reindexer change:
#1728

Co-authored-by: John Kerl <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants