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

[r] Replace SOMAArray read and write calls with ManagedQuery #3678

Merged
merged 5 commits into from
Feb 7, 2025

Conversation

nguyenv
Copy link
Member

@nguyenv nguyenv commented Feb 5, 2025

Issue and/or context:

Part of work for #2054

Changes:

  • Change C++ ManagedQuery constructor to take in SOMAArray instead of std::unique_ptr<SOMAArray>. Pybind11 by default holds a unique_ptr (shared_ptr can be used, but there are known issues which prevents us from using it) whereas Rcpp holds a shared_ptr - to easily support both, pass by value rather than smart pointer
  • Refactor tiledbsoma-r to use ManagedQuery for read and write queries
  • Rename sr_{setup,next,etc} to mq_{setup,next,etc} where mq_setup now returns a ManagedQuery (rather than SOMAArray) that can be iterated through with sr_next
  • Update apply_dim_{points,ranges} utility functions to take in a ManagedQuery

Notes for Reviewer:

Planned next steps:

  1. Remove read, write, etc. from SOMAArray in C++ API.
  2. Currently ManagedQuery::submit_write always calls submit and finalize. Rename ManagedQuery::submit_read to ManagedQuery::submit which both read and write path can use. Create a separate ManagedQuery::finalize.
  3. Refactor the tiledbsoma-py write paths so that we only finalize once at the end, even with multiple submits.

Copy link

codecov bot commented Feb 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.33%. Comparing base (4bcda05) to head (88fc315).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3678      +/-   ##
==========================================
+ Coverage   86.29%   86.33%   +0.04%     
==========================================
  Files          55       55              
  Lines        6390     6390              
==========================================
+ Hits         5514     5517       +3     
+ Misses        876      873       -3     
Flag Coverage Δ
python 86.33% <ø> (+0.04%) ⬆️

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

Components Coverage Δ
python_api 86.33% <ø> (+0.04%) ⬆️
libtiledbsoma ∅ <ø> (∅)

@nguyenv nguyenv marked this pull request as ready for review February 6, 2025 21:01
Copy link
Member

@mojaveazure mojaveazure left a comment

Choose a reason for hiding this comment

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

@nguyenv nguyenv force-pushed the viviannguyen/r-use-managedquery branch from be846e5 to 88fc315 Compare February 6, 2025 21:49
@nguyenv nguyenv merged commit 2105abc into main Feb 7, 2025
28 checks passed
@nguyenv nguyenv deleted the viviannguyen/r-use-managedquery branch February 7, 2025 02:12
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.

2 participants