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

[python] Implement IntIndexer as class that wraps around clib.IntIndexer #2310

Merged
merged 6 commits into from
Mar 26, 2024

Conversation

jp-dark
Copy link
Collaborator

@jp-dark jp-dark commented Mar 22, 2024

Issue and/or context: #2187

Changes:

  • Replace the tiledbsoma_build_index function with a new IntIndex class that wraps clib.IntIndexer.
  • Move type checking from PyBind interface in the clib.IntIndexer class to the python layer using isinstance.

Notes for Reviewer:

Required a bump in somacore to relax the requirements for an IndexLike class.

@jp-dark jp-dark requested a review from nguyenv March 22, 2024 21:55
Copy link

codecov bot commented Mar 25, 2024

Codecov Report

Merging #2310 (a18d42b) into main (9d8a8fb) will decrease coverage by 0.14%.
Report is 1 commits behind head on main.
The diff coverage is 95.23%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2310      +/-   ##
==========================================
- Coverage   78.88%   78.75%   -0.14%     
==========================================
  Files         140      140              
  Lines       10748    10753       +5     
  Branches      213      215       +2     
==========================================
- Hits         8479     8468      -11     
- Misses       2171     2186      +15     
- Partials       98       99       +1     
Flag Coverage Δ
libtiledbsoma 67.56% <ø> (-0.61%) ⬇️
python 90.58% <95.23%> (+<0.01%) ⬆️
r 74.69% <ø> (ø)

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

Components Coverage Δ
python_api 90.58% <95.23%> (+<0.01%) ⬆️
libtiledbsoma 48.69% <ø> (-1.03%) ⬇️

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.

Thank you so much! Looks overall good to me. Just some minor comments.

apis/python/src/tiledbsoma/_indexer.py Outdated Show resolved Hide resolved
apis/python/src/tiledbsoma/_indexer.py Outdated Show resolved Hide resolved
apis/python/src/tiledbsoma/reindexer.cc Outdated Show resolved Hide resolved
apis/python/src/tiledbsoma/reindexer.cc Outdated Show resolved Hide resolved
apis/python/tests/test_reindexer_api.py Outdated Show resolved Hide resolved
@jp-dark
Copy link
Collaborator Author

jp-dark commented Mar 26, 2024

[sc-43678]

Copy link

@jp-dark jp-dark merged commit bc1cd79 into main Mar 26, 2024
23 checks passed
@jp-dark jp-dark deleted the dark/intindexer branch March 26, 2024 17:08
Copy link

The backport to release-1.8 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-release-1.8 release-1.8
# Navigate to the new working tree
cd .worktrees/backport-release-1.8
# Create a new branch
git switch --create backport-2310-to-release-1.8
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick --mainline 1 bc1cd79abadfaba9f8a3d7e81028187857dd9ed8
# Push it to GitHub
git push --set-upstream origin backport-2310-to-release-1.8
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-release-1.8

Then, create a pull request where the base branch is release-1.8 and the compare/head branch is backport-2310-to-release-1.8.

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