-
Notifications
You must be signed in to change notification settings - Fork 27
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] SOMASparseNDArray: sparseMatrix zero-based shim to facilitate soma_joinid lookup #1313
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #1313 +/- ##
===========================================
- Coverage 65.52% 53.48% -12.04%
===========================================
Files 97 61 -36
Lines 7768 5175 -2593
===========================================
- Hits 5090 2768 -2322
+ Misses 2678 2407 -271
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@eddelbuettel Updated NAMESPACE and added the Rd's for the new methods. Perhaps we leave updating the existing Rd's to a followup since, as you noted, it's a lot of unrelated changes. |
@@ -1,8 +1,12 @@ | |||
# Generated by roxygen2: do not edit by hand | |||
|
|||
S3method("[",matrixZeroBasedView) | |||
S3method("[<-",matrixZeroBasedView) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good to go.
@eddelbuettel thanks! @mojaveazure @aaronwolen double-checking just since this was such a tricky topic -- any last comments/concerns? |
Extended discussion: #1232
To load a one-based
Matrix::sparseMatrix
with the contents of a zero-basedSOMASparseNDArray
, we add one to the offsets on each dimension. However, these dimensions are usually populated withsoma_joinid
intended to match thesoma_joinid
column in theobs
&var
data frames, and shifting them by one makes this join operation very error-prone.Here we introduce a minimal shim for
sparseMatrix
that provides matrix access with zero-based indexes. To make this explicit for the user, we renameSOMASparseNDArray$read_sparse_matrix()
toSOMASparseNDArray$read_sparse_matrix_zero_based()
. The shim supports only minimal access operations, which is intentional to prevent "mixing" it with conventional one-based objects. If needed, the fully-featuredsparseMatrix
is recovered by callingas.one.based()
on the shim. Thus, the default behavior is to matchsoma_joinid
but an advanced user can explicitly change to one-based indexing for further use in R.This is a refinement of the first attempt #1306 which was far more complex and error-prone, attempting a subclass with selected methods overridden instead of the distinct wrapper shim shown here.