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

[c++] Fix upgrade-soma-joinid-shape for dataframes having non-standard dimensions #3416

Merged
merged 3 commits into from
Dec 11, 2024

Conversation

johnkerl
Copy link
Member

@johnkerl johnkerl commented Dec 10, 2024

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

Changes:

There should be a closing curly brace here:


and not here:

As this correctly describes

// For upgrade: copy from the full/wide/max domain except for the
// soma_joinid restriction.

we need to be copying from core domain (soma maxdomain) to core current domain (soma domain) for all the other dims.

One problem is that the only remaining code which tests this (now that we aren't creating new arrays without current domain anymore) is in

def test_canned_experiments(tmp_path, has_shapes):

-- and the "canned" (saved-off) pre-1.15 xexperiments only have dataframes that are 'standard' i.e. soma_joinid is their only dim.

i'll add some pre-1.15 data to test this.

Notes for Reviewer:

When this PR is ready for review, please review it with
https://github.com/single-cell-data/TileDB-SOMA/pull/3416/files?diff=split&w=1 (edited)
as that will not flag whitespace as red/green, and this PR is (a) just moving a curly brace, with concomitant indentation change, and (b) additional unit-test coverage.

Copy link

codecov bot commented Dec 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.09%. Comparing base (cbfa704) to head (e048cce).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3416      +/-   ##
==========================================
+ Coverage   85.99%   86.09%   +0.09%     
==========================================
  Files          55       55              
  Lines        6221     6221              
==========================================
+ Hits         5350     5356       +6     
+ Misses        871      865       -6     
Flag Coverage Δ
python 86.09% <ø> (+0.09%) ⬆️

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

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

@johnkerl johnkerl force-pushed the kerl/sdf-non-std-resize branch from 478359f to 1521201 Compare December 10, 2024 23:25
@johnkerl johnkerl marked this pull request as ready for review December 10, 2024 23:38
@johnkerl johnkerl changed the title [c++] Fix upgrade-shape for dataframes with non-standard dimensions [c++] Fix upgrade-soma-joinid-shape for dataframes with non-standard dimensions Dec 10, 2024
@johnkerl johnkerl changed the title [c++] Fix upgrade-soma-joinid-shape for dataframes with non-standard dimensions [c++] Fix upgrade-soma-joinid-shape for dataframes having non-standard dimensions Dec 10, 2024
@johnkerl johnkerl merged commit 00163a7 into main Dec 11, 2024
15 checks passed
@johnkerl johnkerl deleted the kerl/sdf-non-std-resize branch December 11, 2024 01:59
github-actions bot pushed a commit that referenced this pull request Dec 11, 2024
…d dimensions (#3416)

* [c++] Fix upgrade-shape for dataframes with non-standard dimensions

* unit testing

* code-review feedback
johnkerl added a commit that referenced this pull request Dec 11, 2024
…d dimensions (#3416) (#3419)

* [c++] Fix upgrade-shape for dataframes with non-standard dimensions

* unit testing

* code-review feedback

Co-authored-by: John Kerl <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants