fix: #1162; preserving order of obs after _filter_table_by_elements - #1193
fix: #1162; preserving order of obs after _filter_table_by_elements#1193LucaMarconato wants to merge 13 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1193 +/- ##
==========================================
+ Coverage 92.40% 92.44% +0.03%
==========================================
Files 52 52
Lines 7872 7886 +14
==========================================
+ Hits 7274 7290 +16
+ Misses 598 596 -2
🚀 New features to boost your workflow:
|
|
For the maintainers: I created and added the label "todo-add-extra-comments-on-release-notes", so when we auto-generate release notes we remember to add a comment mentioning potential order implications to users. |
…1162) Move the row-order fix from `_filter_table_by_elements` into `_left_join_spatialelement_table`. Our left_join with match `match_rows="no"` is actually a semi-join for the table -- exactly what we need for filter. A semi-join should preserve order, so fixing it there. ( By sorting the table indices, already used for subsetting after all grouped operations. (which caused the reordering).
jan-glx
left a comment
jan-glx
left a comment
There was a problem hiding this comment.
Looks correct, but I think now upstream fix is the way to go.
| assert list(sdata["table"].obs.columns) == ["region", "instance_id", "label"] | ||
|
|
||
|
|
||
| def test_filter_table_non_annotating(full_sdata): |
There was a problem hiding this comment.
I expanded them, please can you re-review?
| del table.obs[order_col] | ||
| if filtered is None or len(filtered) == 0: | ||
| return None | ||
| filtered = filtered[np.argsort(filtered.obs[order_col].to_numpy()), :].copy() |
There was a problem hiding this comment.
Looks correct, but probably upstream fix is more efficient (no double copy)
There was a problem hiding this comment.
Thanks for the upstream fix; I extended it to the inner join.
|
In d61c0ca I proposed an alternative solution that restores the row order for the table already in the semi join performed in |
Fix the row-order scrambling in `_left_join_spatialelement_table`: the join groups matching table rows by region, which does not preserve `table.obs` order when a table annotates multiple interleaved regions. For `match_rows="no"` (a semi-join) there is no element-driven ordering to honor, so restore the original table row order at the source.
d61c0ca to
1e3c764
Compare
…ub.com/scverse/spatialdata into fix/reorder-obs-after-relational-query
| if match_rows == "right": | ||
| warnings.warn("Matching rows 'right' is not supported for 'left' join.", UserWarning, stacklevel=2) | ||
| warnings.warn( | ||
| "Matching rows 'right' is not supported for 'left' join; it will be treated as 'no'.", |
There was a problem hiding this comment.
Truth be told, for left join we could support the right match_rows, and for right join we could support the left match_rows. But we can skip it for now (since also it was not supported before this PR), and eventually do it in the future.
For left_exclusive, right match_rows does not make sense, so it is not supported. Same for right_exclusive: left match_rows does not make sense there.
…ub.com/scverse/spatialdata into fix/reorder-obs-after-relational-query
…nd right join, with match_rows=no)
| # TODO: make this test more interesting by adding indices 5, 4 to "a" and 4, 6 to "b" | ||
| # by design, "left_exclusive" never returns a table (only filtered elements), regardless of | ||
| # match_rows or whether anything was actually excluded. |
There was a problem hiding this comment.
We can do in this PR or leave for the future. It is beyond the scope of the original fix anyway.
|
Thanks @jan-glx for the review. I upstreamed the fix as suggested and added extensive tests for all the join and match_rows combinations for tables with interleaved annotations of the elements. In doing so I found out that:
I kindly ask you for a re-review. |
3 participants
Closes #1162