Skip to content

[c++/python] Map core-to-soma domains correctly#2957

Merged
johnkerl merged 3 commits intomainfrom
kerl/sdf-domain-accessors
Sep 6, 2024
Merged

[c++/python] Map core-to-soma domains correctly#2957
johnkerl merged 3 commits intomainfrom
kerl/sdf-domain-accessors

Conversation

@johnkerl
Copy link
Copy Markdown
Contributor

@johnkerl johnkerl commented Sep 5, 2024

Issue and/or context: As tracked on issue #2407 / [sc-51048].

Note that the intended Python and R API changes are all agreed on and finalized as described in #2407.

Changes:

  • Exposed SOMA-level domain and maxdomain to Python
  • Reduce name-confusion in C++ via explicit and well-commented delgation

Notes for Reviewer:

There is no R counterpart on this PR (compare #2951 which is at parity):

  • TileDB-SOMA-R has no domain accessor for arrays, and so nothing to put a maxdomain next to
  • TileDB-SOMA-R is still going through TileDB-R, not libtiledbsoma, for its $dimensions() accessor
  • Also note (relatedly) TileDB-SOMA-R doesn't support the domain argument to SOMADataFrame's create as TileDB-SOMA-Py does -- I'll file a tracking issue
> sdf <- SOMAOpen('/var/p/obs')

> sdf$ TAB-COMPLETION
sdf$.__enclos_env__              sdf$dimensions                   sdf$ndim                         sdf$soma_type
sdf$.maybe_soma_joinid_maxshape  sdf$dimnames                     sdf$non_empty_domain             sdf$tiledb_array
sdf$.maybe_soma_joinid_shape     sdf$exists                       sdf$object                       sdf$tiledb_schema
sdf$.tiledb_timestamp_range      sdf$fragment_count               sdf$open                         sdf$tiledb_timestamp
sdf$attributes                   sdf$get_metadata                 sdf$platform_config              sdf$tiledbsoma_ctx
sdf$attrnames                    sdf$has_upgraded_domain          sdf$print                        sdf$update
sdf$class                        sdf$index_column_names           sdf$read                         sdf$uri
sdf$clone                        sdf$initialize                   sdf$reopen                       sdf$used_shape
sdf$close                        sdf$is_open                      sdf$schema                       sdf$write
sdf$colnames                     sdf$maxshape                     sdf$set_metadata
sdf$create                       sdf$mode                         sdf$shape

> sdf$dimensions()
$soma_joinid
tiledb_dim(name="soma_joinid", domain=c(0L,2147483646L), tile=2048L, type="INT64", filter_list=tiledb_filter_list(c(tiledb_filter_set_option(tiledb_filter("ZSTD"),"COMPRESSION_LEVEL",3))))

@johnkerl johnkerl requested a review from nguyenv September 5, 2024 19:42
@johnkerl johnkerl force-pushed the kerl/sdf-domain-accessors branch from 7f311e7 to 1ca54fd Compare September 5, 2024 19:43
@johnkerl johnkerl changed the title [c++] Map core-to-soma domains correctly [c++/python] Map core-to-soma domains correctly Sep 5, 2024
@johnkerl johnkerl force-pushed the kerl/sdf-domain-accessors branch 2 times, most recently from a671495 to d142e2d Compare September 5, 2024 21:42
Base automatically changed from kerl/py-r-accessor-plumbing to main September 5, 2024 21:59
@jdblischak jdblischak mentioned this pull request Sep 6, 2024
Closed
@johnkerl johnkerl force-pushed the kerl/sdf-domain-accessors branch from d142e2d to 8a2a9f6 Compare September 6, 2024 14:27
@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 6, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 70.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 89.99%. Comparing base (796c168) to head (7a397c4).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2957      +/-   ##
==========================================
+ Coverage   89.89%   89.99%   +0.09%     
==========================================
  Files          39       39              
  Lines        4049     4057       +8     
==========================================
+ Hits         3640     3651      +11     
+ Misses        409      406       -3
Flag Coverage Δ
python 89.99% <70.00%> (+0.09%) ⬆️

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

Components Coverage Δ
python_api 89.99% <70.00%> (+0.09%) ⬆️
libtiledbsoma ∅ <ø> (∅)

nguyenv
nguyenv approved these changes Sep 6, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@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 the well written documentation

Comment thread libtiledbsoma/src/soma/soma_array.h
@johnkerl
code-review feedbcak
374bfb1 
Co-authored-by: nguyenv <[email protected]>
@johnkerl
Copy link
Copy Markdown
Contributor Author

johnkerl commented Sep 6, 2024

Thanks for the well written documentation

You're welcome! The core-to-soma mapping is absoutely the central part of this project, and the names can get very confusing -- so I believed it was well worth the time to clarify terminology. :)

@johnkerl johnkerl force-pushed the kerl/sdf-domain-accessors branch from a5b330e to 7a397c4 Compare September 6, 2024 17:34
@johnkerl johnkerl merged commit 778bfcc into main Sep 6, 2024
@johnkerl johnkerl deleted the kerl/sdf-domain-accessors branch September 6, 2024 18:45
@johnkerl johnkerl mentioned this pull request Sep 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@nguyenv nguyenv nguyenv approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@johnkerl @nguyenv