Uh oh!
There was an error while loading. Please reload this page.
fix: #1162; preserving order of obs after _filter_table_by_elements - #1193
Conversation
LucaMarconato
commented
Aug 25, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## main #1193 +/- ##
==========================================
+ Coverage 91.85% 91.89% +0.03%
==========================================
Files 53 53 Lines 7928 7942 +14 ==========================================
+ Hits 7282 7298 +16 + Misses 646 644 -2
🚀 New features to boost your workflow:
|
LucaMarconato
commented
Aug 25, 2026
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. |
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| 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.
jan-glx
commented
Aug 25, 2026
In d61c0ca I proposed an alternative solution that restores the row order for the table already in the semi join performed in |
d61c0ca to
1e3c764Compare| 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.
Uh oh!
There was an error while loading. Please reload this page.
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. |
jan-glx
commented
Aug 28, 2026
jan-glx
commented
Aug 28, 2026
I also checked that, on pre-fix code, the extended test suite:
After applying the 3-line |
d358535 to
20744c1Comparejan-glx
commented
Aug 28, 2026
(rewrote clean history, same end result) |
Adds a fixture with a table annotating multiple interleaved regions (_make_interleaved_regions_sdata) and a test parametrized over every `how` / match_rows combination of join_spatialelement_table, asserting the returned table and spatial element row orders and any expected "match_rows not supported" warning. Written against the intended (fixed) behavior, so several cases are expected to fail against the current, unfixed relational_query.py: the semi-join row order (#1162), "inner"/"right" join element order, and the match_rows fallback warnings not actually taking effect.
…ture Extends _make_interleaved_regions_sdata: a "c" region present only in the table (no corresponding spatial element), and instance ids in "a"/"b" with no matching table row. The former gives right_exclusive joins a row to exclude on; the latter lets "left" and "left_exclusive" joins be checked against unmatched element rows too, not just already-matched ones.
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.
…e (inner and right join, with match_rows=no)
_get_masked_element() only reordered the masked element to follow
element_indices for match_rows in {"left", "right"}; for the default
match_rows="no" it left rows in table order instead of the element's own
order, affecting "inner" and "right" joins.
"inner" join grouped matching table rows by region, which does not
preserve the original table.obs row order when a table annotates multiple
interleaved regions, for both match_rows="no" and match_rows="right" (the
table's own order takes priority in both cases)._left_exclusive_join_spatialelement_table() masked by treating the table's instance ids as positional indices into the element instead of as index labels, which is wrong whenever the element's index isn't a default 0..n-1 range or doesn't contain every instance id for the region.
"right_exclusive", "right" and "left" joins warned that an unsupported
match_rows value ("left" or "right", depending on the join) would be
"treated as 'no'", but never actually reassigned match_rows to "no" — so
the join kept using the unsupported value instead of falling back as
described. Also improves the two pre-existing warning messages to match
the wording used elsewhere ("...; it will be treated as 'no'.") and adds
the same guard to "right_exclusive", which previously had none at all.right_exclusive drops unmatched table rows belonging to a region with no queried spatial element (the "c7" row). Marking it xfail(strict=True) so the still-open bug is documented and the suite stays green, while forcing an error (and prompting marker removal) once it's fixed.
20744c1 to
cf97dbcCompareUh oh!
There was an error while loading. Please reload this page.
Closes#1162