Skip to content

[r] Implement missing domain argument to DataFrame create#3032

Merged
johnkerl merged 9 commits intomainfrom
kerl/sdf-domain-at-create
Sep 30, 2024
Merged

[r] Implement missing domain argument to DataFrame create#3032
johnkerl merged 9 commits intomainfrom
kerl/sdf-domain-at-create

Conversation

@johnkerl
Copy link
Copy Markdown
Contributor

@johnkerl johnkerl commented Sep 22, 2024 •
edited
Loading

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

Changes:

As a legacy R/Python-parity issue, the domain argument to SOMADataFrame's create -- from the SOMA spec -- was never implemented in R. Here we remedy that.

Notes for Reviewer:

This PR is a work in progress. It is not ready for review.

Unit-test cases are not yet implemented.

@johnkerl johnkerl mentioned this pull request Sep 22, 2024
Closed
@johnkerl johnkerl force-pushed the kerl/sdf-domain-at-create branch from fb3bebb to 12cc969 Compare September 22, 2024 22:07
@johnkerl johnkerl changed the base branch from main to kerl/hll-domainish September 22, 2024 22:07
@johnkerl johnkerl force-pushed the kerl/sdf-domain-at-create branch 6 times, most recently from 4811339 to 50e7a5d Compare September 22, 2024 23:01
@johnkerl johnkerl force-pushed the kerl/sdf-domain-at-create branch 4 times, most recently from 248ba3a to d4060ad Compare September 25, 2024 04:09
@johnkerl johnkerl force-pushed the kerl/sdf-domain-at-create branch from d4060ad to 4900762 Compare September 25, 2024 04:58
@johnkerl johnkerl marked this pull request as ready for review September 25, 2024 04:59
@johnkerl johnkerl force-pushed the kerl/sdf-domain-at-create branch from 4900762 to 3ab1c7d Compare September 25, 2024 13:18
@johnkerl johnkerl requested a review from nguyenv September 25, 2024 14:05
@johnkerl johnkerl force-pushed the kerl/sdf-domain-at-create branch 2 times, most recently from df2dc41 to 4f15cb2 Compare September 25, 2024 18:18
@johnkerl
Copy link
Copy Markdown
Contributor Author

johnkerl commented Sep 25, 2024 •
edited
Loading

I have one more bug to fix in CI -- it's a KP 🙏

Update: fixed

@johnkerl johnkerl mentioned this pull request Sep 25, 2024
Merged
@johnkerl johnkerl marked this pull request as draft September 25, 2024 22:09
@johnkerl johnkerl changed the title [r] Implement missing domain argument to DataFrame create [r] Implement missing domain argument to DataFrame create [WIP] Sep 25, 2024
@johnkerl johnkerl force-pushed the kerl/sdf-domain-at-create branch from 4f15cb2 to 28a00e6 Compare September 25, 2024 22:23
@johnkerl johnkerl force-pushed the kerl/sdf-domain-at-create branch from 28a00e6 to c684adb Compare September 25, 2024 23:41
@johnkerl johnkerl force-pushed the kerl/sdf-domain-at-create branch from feaaa50 to 877ec91 Compare September 27, 2024 00:58
@johnkerl johnkerl mentioned this pull request Sep 27, 2024
Merged
Base automatically changed from kerl/hll-domainish to main September 27, 2024 13:14
@johnkerl johnkerl force-pushed the kerl/sdf-domain-at-create branch 2 times, most recently from 25cbbe1 to 3e731f8 Compare September 27, 2024 15:00
@johnkerl johnkerl mentioned this pull request Sep 27, 2024
Merged
@johnkerl johnkerl force-pushed the kerl/sdf-domain-at-create branch from 3e731f8 to 7dc11ab Compare September 27, 2024 20:42
@johnkerl johnkerl mentioned this pull request Sep 28, 2024
Merged
@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 30, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 88.98%. Comparing base (2f6c442) to head (1531119).
Report is 5 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3032      +/-   ##
==========================================
- Coverage   89.02%   88.98%   -0.04%     
==========================================
  Files          45       45              
  Lines        4284     4287       +3     
==========================================
+ Hits         3814     3815       +1     
- Misses        470      472       +2
Flag Coverage Δ
python 88.98% <50.00%> (-0.04%) ⬇️

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

Components Coverage Δ
python_api 88.98% <50.00%> (-0.04%) ⬇️
libtiledbsoma ∅ <ø> (∅)

@johnkerl johnkerl force-pushed the kerl/sdf-domain-at-create branch from 1531119 to 27dc91f Compare September 30, 2024 18:57
mojaveazure
mojaveazure requested changes Sep 30, 2024
View reviewed changes
Comment thread apis/r/R/SOMADataFrame.R Outdated
Comment thread apis/r/R/utils-arrow.R Outdated
Comment thread apis/r/R/utils-arrow.R Outdated
Comment thread apis/r/R/utils-arrow.R Outdated
Comment thread apis/r/tests/testthat/test-shape.R Outdated
Comment thread apis/r/tests/testthat/test-shape.R
Comment thread apis/r/tests/testthat/test-shape.R Outdated
Comment thread apis/r/tests/testthat/test-shape.R Outdated
Comment thread apis/r/R/Factory.R Outdated
Comment thread apis/r/R/SOMADataFrame.R Outdated
@johnkerl johnkerl force-pushed the kerl/sdf-domain-at-create branch from 9269614 to 39db38b Compare September 30, 2024 22:00
@johnkerl johnkerl requested review from mojaveazure and removed request for nguyenv September 30, 2024 22:09
mojaveazure
mojaveazure requested changes Sep 30, 2024
View reviewed changes
Copy link
Copy Markdown
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.

There was one comment on my last review that didn't get looked at https://github.com/single-cell-data/TileDB-SOMA/pull/3032/files#r1781709758

@johnkerl @mojaveazure
code-review feedback
e7a2e74 
Co-authored-by: Paul Hoffman <[email protected]>
@johnkerl
Copy link
Copy Markdown
Contributor Author

johnkerl commented Sep 30, 2024

Oops, my bad @mojaveazure ! e7a2e74

mojaveazure
mojaveazure approved these changes Sep 30, 2024
View reviewed changes
Copy link
Copy Markdown
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.

🚢

@johnkerl johnkerl merged commit 8885e31 into main Sep 30, 2024
@johnkerl johnkerl deleted the kerl/sdf-domain-at-create branch September 30, 2024 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mojaveazure mojaveazure mojaveazure approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[r] SOMADataFrame create needs to accept a domain argument

2 participants

@johnkerl @mojaveazure