-
Notifications
You must be signed in to change notification settings - Fork 47
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
refactor filter_table_by_elements
#701
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -135,9 +135,15 @@ def test_filter_by_coordinate_system(full_sdata: SpatialData) -> None: | |
def test_filter_by_coordinate_system_also_table(full_sdata: SpatialData) -> None: | ||
from spatialdata.models import TableModel | ||
|
||
rng = np.random.default_rng(seed=0) | ||
full_sdata["table"].obs["annotated_shapes"] = rng.choice(["circles", "poly"], size=full_sdata["table"].shape[0]) | ||
adata = full_sdata["table"] | ||
adata = full_sdata["table"].copy() | ||
|
||
circles_instances = full_sdata["circles"].index.values | ||
poly_instances = full_sdata["poly"].index.values | ||
|
||
adata = adata[: len(circles_instances) + len(poly_instances), :].copy() | ||
adata.obs["annotated_shapes"] = ["circles"] * len(circles_instances) + ["poly"] * len(poly_instances) | ||
adata.obs["instance_id"] = np.concatenate([circles_instances, poly_instances]) | ||
|
||
Comment on lines
-138
to
+146
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this test had quite a big bug. Basically, the table natively from conftest annotates labels, but here it was re used to annotate shapes and circles. Now, both shapes and circles have 5 instances only, and so the table was being filtered only by coordinate system, but this meant that the table had the first five instances mapping to the poly/circles, but then all the other instances also present, which did not map to anything. This is because the filtering was happening with the now removed I will add test so that the filter_table function always return correct tables. |
||
del adata.uns[TableModel.ATTRS_KEY] | ||
del full_sdata.tables["table"] | ||
full_sdata.table = TableModel.parse( | ||
|
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.
this function is buggy, and it doesn't make sense with the new multiple table design, as it can return wrong tables. I removed it and refactored it in the other filter method. I will add test for this. See other comment