-
Notifications
You must be signed in to change notification settings - Fork 29
[python] Add cell and transcript ingestion for Xenium #3893
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
Conversation
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 overall looks good. I have a couple small suggestions, mostly nits.
I'm going to wait to approve it until the PR #3835 gets merged to main so we don't accidentally combine them when we squash the PR commits.
if isinstance(platform_config, dict): | ||
platform_config.setdefault("tiledb", dict()) | ||
if isinstance(platform_config["tiledb"], dict): | ||
platform_config["tiledb"].setdefault("create", dict()) | ||
if isinstance(platform_config["tiledb"]["create"], dict): | ||
platform_config["tiledb"]["create"].setdefault( | ||
"allows_duplicates", True | ||
) |
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.
I think you want to get rid of the isinstance
checks in here. I might be mistaken, but I think they should always evaluate to True
since you setdefault
first. If not, then you are failing to set allows_duplicates
to True
by default.
return xenium_path | ||
|
||
|
||
@pytest.mark.skip(reason="Missing test dataset") |
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.
We need to add the test data to the test data repo before we fully release the Xenium reader. I added a shortcut story for this.
@@ -8,7 +8,9 @@ namespace tiledbsoma { | |||
|
|||
class OutlineTransformer : public Transformer { | |||
public: | |||
OutlineTransformer(SOMACoordinateSpace coordinate_space); | |||
OutlineTransformer( |
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.
Could you add some docs either here or above the columns
private member on what columns
is and how it is used?
eedcff4
to
71cf0ee
Compare
4a7c8d8
to
33f79e9
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3893 +/- ##
===========================================
- Coverage 89.18% 66.29% -22.90%
===========================================
Files 57 156 +99
Lines 6901 20627 +13726
Branches 0 1205 +1205
===========================================
+ Hits 6155 13674 +7519
- Misses 746 6543 +5797
- Partials 0 410 +410
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
from .ingest import ( | ||
VisiumPaths, | ||
XeniumPaths, | ||
from_visium, | ||
from_xenium, | ||
register_visium_datasets, | ||
) | ||
from .outgest import to_spatialdata | ||
|
||
__all__ = ["to_spatialdata", "from_visium", "register_visium_datasets", "VisiumPaths"] | ||
__all__ = [ | ||
"to_spatialdata", | ||
"from_visium", | ||
"register_visium_datasets", | ||
"VisiumPaths", | ||
"from_xenium", | ||
"XeniumPaths", | ||
] |
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.
Let's not add from_xenium
and XeniumPaths
to the module until it's fully implemented.
De-prioritized for now. We can re-open later when we have more resources to dedicate towards spatial io. |
Issue and/or context: sc-52730
Changes:
Notes for Reviewer: