Skip to content

[python/r] DataFrame resizer#3091

Merged
johnkerl merged 7 commits intomainfrom
kerl/maybe-resize-soma-joinid-py-r
Oct 1, 2024
Merged

[python/r] DataFrame resizer#3091
johnkerl merged 7 commits intomainfrom
kerl/maybe-resize-soma-joinid-py-r

Conversation

@johnkerl
Copy link
Copy Markdown
Contributor

@johnkerl johnkerl commented Sep 28, 2024 •
edited
Loading

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:

While the full-generality upgrade_domain is on track for #2407, the most important special case is resizing the soma_joinid index column and leaving the others alone, if soma_joinid is an index column -- otherwise leaving it alone.

This already exists in C++ and is well-tested there. This PR simply connects it to Python/R. It's time for this, as we need:

Notes for Reviewer:

  • This PR is stacked on top of [c++] Simplify internal API for dataframe-resizer #3090
  • This PR isn't super-big -- 200 lines total as I write this -- nonetheless it's multi-language
    • I prefer this TBH for Python/R parity whenever possible
    • Also the commits are carefully split out, so a commit-at-a-time review may be helpful (up to you of course)

@johnkerl johnkerl mentioned this pull request Sep 28, 2024
Closed
@johnkerl johnkerl force-pushed the kerl/maybe-resize-soma-joinid-cpp-tweak branch from 0178ab5 to 7981d82 Compare September 28, 2024 21:49
@johnkerl johnkerl force-pushed the kerl/maybe-resize-soma-joinid-py-r branch from 0ca52b0 to 49a2b21 Compare September 28, 2024 21:49
@johnkerl johnkerl force-pushed the kerl/maybe-resize-soma-joinid-py-r branch from 9c64a11 to a66595a Compare September 29, 2024 04:26
@johnkerl johnkerl force-pushed the kerl/maybe-resize-soma-joinid-cpp-tweak branch from 7981d82 to 96f70e1 Compare September 30, 2024 16:59
nguyenv
nguyenv reviewed Sep 30, 2024
View reviewed changes
Comment thread apis/python/src/tiledbsoma/soma_dataframe.cc
nguyenv
nguyenv approved these changes Sep 30, 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.

Look good on the Python side. Just one minor comment.

@johnkerl
Copy link
Copy Markdown
Contributor Author

johnkerl commented Sep 30, 2024

Thanks @nguyenv !

@johnkerl johnkerl force-pushed the kerl/maybe-resize-soma-joinid-cpp-tweak branch from 96f70e1 to 3258070 Compare September 30, 2024 17:48
Base automatically changed from kerl/maybe-resize-soma-joinid-cpp-tweak to kerl/sdf-domain-at-create September 30, 2024 18:17
@johnkerl johnkerl force-pushed the kerl/sdf-domain-at-create branch from 1531119 to 27dc91f Compare September 30, 2024 18:57
@johnkerl johnkerl force-pushed the kerl/maybe-resize-soma-joinid-py-r branch from 6692a88 to eeade29 Compare September 30, 2024 18:58
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.

LGTM from R, a few minor comments to be a bit more R-tastic but nothing major

Comment thread apis/r/R/SOMADataFrame.R
Comment thread apis/r/tests/testthat/test-shape.R
Comment thread apis/r/tests/testthat/test-shape.R
@johnkerl johnkerl force-pushed the kerl/sdf-domain-at-create branch from 9269614 to 39db38b Compare September 30, 2024 22:00
@johnkerl johnkerl force-pushed the kerl/maybe-resize-soma-joinid-py-r branch from a418265 to 7fad794 Compare September 30, 2024 22:12
Base automatically changed from kerl/sdf-domain-at-create to main September 30, 2024 23:29
@johnkerl johnkerl force-pushed the kerl/maybe-resize-soma-joinid-py-r branch from e55735b to 2b4d5a0 Compare September 30, 2024 23:30
@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 30, 2024 •
edited
Loading

Codecov Report

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

Project coverage is 62.46%. Comparing base (a162dad) to head (c15a0bb).
Report is 2 commits behind head on main.

Additional details and impacted files 
@@             Coverage Diff             @@
##             main    #3091       +/-   ##
===========================================
- Coverage   88.98%   62.46%   -26.53%     
===========================================
  Files          45      108       +63     
  Lines        4287    13288     +9001     
  Branches        0      825      +825     
===========================================
+ Hits         3815     8301     +4486     
- Misses        472     4788     +4316     
- Partials        0      199      +199
Flag Coverage Δ
libtiledbsoma 49.76% <100.00%> (?)
python 89.09% <66.66%> (+0.10%) ⬆️

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

Components Coverage Δ
python_api 89.09% <66.66%> (+0.10%) ⬆️
libtiledbsoma 48.63% <100.00%> (∅)

@johnkerl johnkerl merged commit 8e9be91 into main Oct 1, 2024
@johnkerl johnkerl deleted the kerl/maybe-resize-soma-joinid-py-r branch October 1, 2024 00:10
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

+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.

3 participants

@johnkerl @nguyenv @mojaveazure