Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 93
fix: #1162; preserving order of obs after _filter_table_by_elements#1193
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
Merged
+231
−4
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
b64160b
test: cover obs/element row-order bugs across join types (#1162)
LucaMarconato d6eb989
test: add region-only-in-table and unmatched-element cases to the fix…
jan-glx 40bed32
fix: preserve obs order in semi-join (#1162)
jan-glx 9a3e529
fix: correct elements and table order for interleaved annotating tabl…
LucaMarconato 492aa6c
fix: correct left_exclusive join to mask by index label, not position
LucaMarconato 380e4b6
fix: make match_rows fallback warnings actually take effect
LucaMarconato cf97dbc
test: xfail right_exclusive cases pending join bug fix
jan-glx File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,9 @@ | ||
| from __future__ import annotations | ||
| import re | ||
| import warnings | ||
| from dataclasses import dataclass, field | ||
| import annsel as an | ||
| import numpy as np | ||
| import pandas as pd | ||
| @@ -940,6 +944,189 @@ def test_filter_table_categorical_bug(shapes): | ||
| shapes.filter_by_coordinate_system("global") | ||
| @dataclass | ||
| class _JoinOutcome: | ||
| """Expected outcome of a `join_spatialelement_table()` call, for a given `how`/`match_rows` pair.""" | ||
| # expected `joined_table.obs["label"]`, in order; `None` when no table is expected to be returned | ||
| table_order: list[str] | None = None | ||
| # whether a "Matching rows '<...>' is not supported for '<...>' join." UserWarning is expected to be emitted | ||
| warns: bool = False | ||
| # expected values of `element_dict[name].index` for element name in {"a", "b"}; `None` for a element name whose | ||
| # returned join result is expected to be `None` (e.g. fully excluded, or not returned by this join type) | ||
| element_index: dict[str, list[int] | None] = field(default_factory=dict) | ||
| def _make_interleaved_regions_sdata() -> tuple[SpatialData, dict[str, dict[str, _JoinOutcome]]]: | ||
| from geopandas import GeoDataFrame | ||
| from shapely.geometry import Point | ||
| from spatialdata.models import ShapesModel | ||
| def circles(indices): | ||
| # `indices` gives both the number of circles and the (non-default) row order of the element. | ||
| gdf = GeoDataFrame( | ||
| {"geometry": [Point(i, i) for i in range(len(indices))], "radius": [1.0] * len(indices)}, | ||
| index=pd.Index(indices), | ||
| ) | ||
| return ShapesModel.parse(gdf) | ||
| # assumptions/comments: | ||
| # - no duplicate values in the index of each spatial element (duplicate values are tested elsewhere) | ||
| # - no duplicate values for the instance_key column of the table (duplicate values are tested elsewhere) | ||
| # | ||
| # edge cases being tested: | ||
| # - instance_id values are non-monotonic | ||
| # - the index in each spatial element is non-monotonic | ||
| # - we also set the index of the table obs to random values; these should be ignored (in the code we call .index on | ||
| # a region_key column, but the index is freshly reset by a nearby call of .reset_index() inside the join | ||
| # machinery) | ||
| # - "b3" and "c7" are unmatched table rows: "b3" refers to a missing instance in a | ||
| # spatial element that exists, while "c7" refers to a region with no spatial element | ||
| obs = pd.DataFrame( | ||
| { | ||
| "region": pd.Categorical(["b", "b", "a", "b", "a", "a", "b", "c"]), | ||
| "instance_id": [2, 1, 2, 3, 1, 0, 0, 7], | ||
| "label": ["b2", "b1", "a2", "b3", "a1", "a0", "b0", "c7"], | ||
| }, | ||
| index=np.random.default_rng(0).integers(0, 3, size=8).astype(str), | ||
| ) | ||
| # "a" additionally has unmatched instance ids 5, 4, and "b" has 4, 6. | ||
| # These test that unmatched element rows are preserved in element order by | ||
| # "left" and "left_exclusive" joins. | ||
| shapes = {"a": circles([2, 1, 0, 5, 4]), "b": circles([1, 2, 0, 4, 6])} | ||
| # to make understanding easier, you may want to refer to the figure on joins from the docs: | ||
| # https://spatialdata.scverse.org/en/stable/tutorials/notebooks/notebooks/examples/tables.html | ||
| expected = { | ||
| "left": { | ||
| "no": _JoinOutcome( | ||
| table_order=["b2", "b1", "a2", "a1", "a0", "b0"], | ||
| element_index={"a": [2, 1, 0, 5, 4], "b": [1, 2, 0, 4, 6]}, | ||
| ), | ||
| "left": _JoinOutcome( | ||
| table_order=["a2", "a1", "a0", "b1", "b2", "b0"], | ||
| element_index={"a": [2, 1, 0, 5, 4], "b": [1, 2, 0, 4, 6]}, | ||
| ), | ||
| "right": _JoinOutcome( | ||
| table_order=["b2", "b1", "a2", "a1", "a0", "b0"], | ||
| warns=True, | ||
| element_index={"a": [2, 1, 0, 5, 4], "b": [1, 2, 0, 4, 6]}, | ||
| ), | ||
| }, | ||
| "left_exclusive": { | ||
| # by design, "left_exclusive" never returns a table (only filtered elements), regardless of | ||
| # match_rows or whether anything was actually excluded. | ||
| "no": _JoinOutcome(table_order=None, element_index={"a": [5, 4], "b": [4, 6]}), | ||
| "left": _JoinOutcome(table_order=None, element_index={"a": [5, 4], "b": [4, 6]}), | ||
| "right": _JoinOutcome(table_order=None, warns=True, element_index={"a": [5, 4], "b": [4, 6]}), | ||
| }, | ||
| "inner": { | ||
| "no": _JoinOutcome( | ||
| table_order=["b2", "b1", "a2", "a1", "a0", "b0"], | ||
| element_index={"a": [2, 1, 0], "b": [1, 2, 0]}, | ||
| ), | ||
| "left": _JoinOutcome( | ||
| table_order=["a2", "a1", "a0", "b1", "b2", "b0"], element_index={"a": [2, 1, 0], "b": [1, 2, 0]} | ||
| ), | ||
| "right": _JoinOutcome( | ||
| table_order=["b2", "b1", "a2", "a1", "a0", "b0"], element_index={"a": [2, 1, 0], "b": [2, 1, 0]} | ||
| ), | ||
| }, | ||
| "right": { | ||
| "no": _JoinOutcome( | ||
| table_order=["b2", "b1", "a2", "b3", "a1", "a0", "b0", "c7"], | ||
| element_index={"a": [2, 1, 0], "b": [1, 2, 0]}, | ||
| ), | ||
| "left": _JoinOutcome( | ||
| table_order=["b2", "b1", "a2", "b3", "a1", "a0", "b0", "c7"], | ||
| warns=True, | ||
| element_index={"a": [2, 1, 0], "b": [1, 2, 0]}, | ||
| ), | ||
| "right": _JoinOutcome( | ||
| table_order=["b2", "b1", "a2", "b3", "a1", "a0", "b0", "c7"], | ||
| element_index={"a": [2, 1, 0], "b": [2, 1, 0]}, | ||
| ), | ||
| }, | ||
| "right_exclusive": { | ||
| "no": _JoinOutcome(table_order=["b3", "c7"], element_index={"a": None, "b": None}), | ||
| "left": _JoinOutcome(table_order=["b3", "c7"], warns=True, element_index={"a": None, "b": None}), | ||
| "right": _JoinOutcome(table_order=["b3", "c7"], element_index={"a": None, "b": None}), | ||
| }, | ||
| } | ||
| table = TableModel.parse( | ||
| AnnData(X=np.zeros((len(obs), 1)), obs=obs), | ||
| region=["a", "b", "c"], | ||
| region_key="region", | ||
| instance_key="instance_id", | ||
| ) | ||
| sdata = SpatialData(shapes=shapes, tables={"table": table}) | ||
| return sdata, expected | ||
| @pytest.mark.parametrize("match_rows", ["no", "left", "right"]) | ||
| @pytest.mark.parametrize( | ||
| "how", | ||
| [ | ||
| "left", | ||
| "left_exclusive", | ||
| "inner", | ||
| "right", | ||
| pytest.param( | ||
| "right_exclusive", | ||
| marks=pytest.mark.xfail( | ||
| reason="known bug (see https://github.com/scverse/spatialdata/issues/1162): 'right_exclusive' join " | ||
| "drops unmatched table rows belonging to a region with no queried spatial element (e.g. 'c7')", | ||
| strict=True, | ||
| ), | ||
| ), | ||
| ], | ||
| ) | ||
| def test_join_preserves_row_order_multiple_interleaved_regions(how, match_rows): | ||
| # generalization to all the join types of the bug reported in https://github.com/scverse/spatialdata/issues/1162 | ||
| # covering all `how` values of `join_spatialelement_table`, crossed with all values of `match_rows`, and checking | ||
| # whether the row orders of the returned spatial elements and table are correct and if the "match_rows not | ||
| # supported" UserWarning is (or isn't) actually raised (see `_make_interleaved_regions_sdata` and `_JoinOutcome`). | ||
| sdata, expected_by_how_and_match_rows = _make_interleaved_regions_sdata() | ||
| outcome = expected_by_how_and_match_rows[how][match_rows] | ||
| with warnings.catch_warnings(record=True) as record: | ||
| warnings.simplefilter("always") | ||
| element_dict, joined_table = join_spatialelement_table( | ||
| sdata=sdata, | ||
| spatial_element_names=["a", "b"], | ||
| table_name="table", | ||
| how=how, | ||
| match_rows=match_rows, | ||
| ) | ||
| # other UserWarnings can also fire here (e.g. anndata's "Observation names are not unique", triggered by | ||
| # the fixture's duplicated obs_names), so only look for the one this test is actually about. The message | ||
| # looks like "Matching rows 'right' is not supported for 'left_exclusive' join; it will be treated as 'no'.", | ||
| # with the two quoted values varying by `match_rows` / `how`. | ||
| unsupported_match_rows_re = re.compile( | ||
| r"Matching rows '[^']+' is not supported for '[^']+' join; it will be treated as 'no'\." | ||
| ) | ||
| unsupported_match_rows_warnings = [ | ||
| w for w in record if issubclass(w.category, UserWarning) and unsupported_match_rows_re.search(str(w.message)) | ||
| ] | ||
| assert bool(unsupported_match_rows_warnings) == outcome.warns | ||
| if outcome.table_order is None: | ||
| assert joined_table is None | ||
| else: | ||
| assert joined_table is not None | ||
| assert list(joined_table.obs["label"]) == outcome.table_order | ||
| for name, expected_index in outcome.element_index.items(): | ||
| actual_element = element_dict[name] | ||
| if expected_index is None: | ||
| assert actual_element is None | ||
| else: | ||
| assert actual_element is not None | ||
| assert list(actual_element.index) == expected_index | ||
| def test_filter_table_non_annotating(full_sdata): | ||
jan-glx marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| obs = pd.DataFrame({"test": ["a", "b", "c"]}, index=list(map(str, range(3)))) | ||
| adata = AnnData(obs=obs) | ||
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.