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

[python/r] DataFrame resizer #3091

Merged
merged 7 commits into from
Oct 1, 2024
Merged

Conversation

johnkerl
Copy link
Member

@johnkerl johnkerl commented Sep 28, 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:

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 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
Copy link
Member

@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
Member Author

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

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

apis/r/R/SOMADataFrame.R Show resolved Hide resolved
apis/r/tests/testthat/test-shape.R Show resolved Hide resolved
apis/r/tests/testthat/test-shape.R Show resolved Hide resolved
@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
Copy link

codecov bot commented Sep 30, 2024

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
15 checks passed
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants