[python/r] Implement resize and tiledbsoma_upgrade_shape#2950
Merged
Conversation
johnkerl
force-pushed
the
kerl/py-r-test-2
branch
16 times, most recently
from
64e885c to
fc64c3f
Compare
johnkerl
force-pushed
the
kerl/py-r-test-2
branch
from
fc64c3f to
70347d1
Compare
johnkerl
force-pushed
the
kerl/sdf-test-accessors
branch
from
5c03e17 to
a40cf77
Compare
johnkerl
force-pushed
the
kerl/py-r-test-2
branch
from
70347d1 to
20b33f1
Compare
johnkerl
force-pushed
the
kerl/sdf-test-accessors
branch
from
a40cf77 to
9ee2797
Compare
johnkerl
force-pushed
the
kerl/py-r-test-2
branch
from
20b33f1 to
dd46b8e
Compare
johnkerl
force-pushed
the
kerl/sdf-test-accessors
branch
from
9ee2797 to
ee19025
Compare
johnkerl
force-pushed
the
kerl/py-r-test-2
branch
4 times, most recently
from
f467719 to
bce3172
Compare
johnkerl
changed the base branch from
kerl/sdf-test-accessors
to
kerl/feature-flag-temp
johnkerl
force-pushed
the
kerl/feature-flag-temp
branch
from
aeb5141 to
8a883be
Compare
johnkerl
force-pushed
the
kerl/py-r-test-2
branch
2 times, most recently
from
b0933d4 to
0f96d28
Compare
johnkerl
changed the base branch from
kerl/feature-flag-temp
to
kerl/py-r-creation-paths
johnkerl
force-pushed
the
kerl/py-r-test-2
branch
4 times, most recently
from
ac08eff to
4bcbae5
Compare
johnkerl
changed the title
resize and upgrade_shape
johnkerl
force-pushed
the
kerl/py-r-creation-paths
branch
from
0b5bddb to
192b63f
Compare
johnkerl
force-pushed
the
kerl/py-r-test-2
branch
from
4bcbae5 to
a8b0b3a
Compare
johnkerl
force-pushed
the
kerl/py-r-creation-paths
branch
from
192b63f to
a735be3
Compare
johnkerl
force-pushed
the
kerl/py-r-test-2
branch
from
a8b0b3a to
04c5c14
Compare
johnkerl
force-pushed
the
kerl/py-r-creation-paths
branch
from
a735be3 to
407fa35
Compare
johnkerl
force-pushed
the
kerl/py-r-test-2
branch
from
04c5c14 to
6e792f9
Compare
johnkerl
commented
Sep 9, 2024
mojaveazure
requested changes
Sep 9, 2024
Contributor
Author
|
@mojaveazure ready for round two! |
mojaveazure
previously requested changes
Sep 11, 2024
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2950 +/- ##
==========================================
+ Coverage 89.73% 89.79% +0.06%
==========================================
Files 39 39
Lines 4080 4096 +16
==========================================
+ Hits 3661 3678 +17
+ Misses 419 418 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Contributor
Author
|
@nguyenv and @mojaveazure ready for the next round! :) |
mojaveazure
approved these changes
Sep 12, 2024
nguyenv
approved these changes
Sep 12, 2024
Contributor
nguyenv
left a comment
nguyenv
left a comment
There was a problem hiding this comment.
lgtm but minor suggestion
Comment on lines
+120
to
+140
| .def( | ||
| "resize", | ||
| [](SOMAArray& array, const std::vector<int64_t>& newshape) { | ||
| try { | ||
| array.resize(newshape); | ||
| } catch (const std::exception& e) { | ||
| throw TileDBSOMAError(e.what()); | ||
| } | ||
| }, | ||
| "newshape"_a) | ||
|
|
||
| .def( | ||
| "tiledbsoma_upgrade_shape", | ||
| [](SOMAArray& array, const std::vector<int64_t>& newshape) { | ||
| try { | ||
| array.upgrade_shape(newshape); | ||
| } catch (const std::exception& e) { | ||
| throw TileDBSOMAError(e.what()); | ||
| } | ||
| }, | ||
| "newshape"_a); |
Contributor
There was a problem hiding this comment.
I think we should do the rethrow in libtiledbsoma rather than pybind11. It'll simplify the code in here to be a one-liner again and the C++ API and R API would also consistently see TileDBSOMAError when erroring out from these methods.
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue and/or context: As tracked on issue #2407 / [sc-51048]. Here we implement
resizeandupgrade_shapefor ND arrays. The dataframe mutators are deferred to a separate PR.Note that the intended Python and R API changes are all agreed on and finalized as described in #2407.
Changes:
Implements and tests the mutators.
Notes for Reviewer:
For a separate PR: needs unit-test cases, including some saved-off arrays (in a tar file, say) lacking the new-shape feature which we can apply upgrade-shape to in unit-test cases.