-
Notifications
You must be signed in to change notification settings - Fork 34
various fixes and additions for IO #19
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
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #19 +/- ##
==========================================
- Coverage 44.90% 42.54% -2.36%
==========================================
Files 11 13 +2
Lines 432 604 +172
==========================================
+ Hits 194 257 +63
- Misses 238 347 +109
|
imread_kwargs, | ||
image_models_kwargs, | ||
) | ||
labels[f"{dataset_id}_nuclei"] = _get_labels( |
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.
Just one note (nothing to do): in the example the labels of the nuclei coincide with the other cell labels.
src/spatialdata_io/readers/visium.py
Outdated
shape_size=scalefactors["spot_diameter_fullres"], | ||
index=adata.obs_names, | ||
geometry=0, | ||
radius=np.repeat(scalefactors["spot_diameter_fullres"], len(adata)), |
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 would modify the parser signature to allow also a single float here. This actually already works because when we assign a float to the column in the dataframe the value is copied for each row.
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.
true it works already, clarified there
adata.obs = metadata | ||
transform = Scale([1.0 / specs["pixel_size"], 1.0 / specs["pixel_size"]], axes=("x", "y")) | ||
diameters = 2 * np.sqrt(adata.obs[XeniumKeys.CELL_AREA].to_numpy() / np.pi) / specs["pixel_size"] | ||
circles = ShapesModel.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.
I am also not a fan of this but I have restored it because this is the only way that we have at the moment to view the data with napari. The polygons are too slow, takes minutes to render the napari view. One option could be to rasterize the polygons into labels. Shall I do it? (I started working on rasterization already so I could code it today)
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 load the circles for the nuclei (not the cell boundaries anymore) otherwise the circles are overlapping in napari.
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.
There was a bug in the radius size, fixed.
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 mean I understand the napari thing but can't it be done temporarily there in case? I don't think it's any useful and it's not really an element provided by the tech but some type of processing, would keep it removed
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 can think of a few alternative options:
- we keep this code in the io until a better solution
- we make the user manually create circles every time he/she is working with Xenium data (in particular the has to manually pass the radius column to the parser of circles)
- I implement the rasterization of polygons to labels and this is called by napari when too many polygons are detected. This function is probably slow, so better if the user calls it beforehand (or even if we call it in
cosmx()
: if I can implement it lazily that would be the best, socosmx()
returns fast and the time is used only when saving to zarr.) - napari shows only the centroids (it can't show the shape sizes since it doesn't know that the radii are stored in a column of the table).
- We make a function to compute the area of polygons and we create a function to create circles with radius from the polygons. As above, we either call this function automatically in napari, either we call it in
cosmx()
, either the user will call it manually if needed.
wdyt?
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.
ok fine for leaving it in, would add an argument like centroids as shapes then to have it at least optional
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.
Yes I would add the arguments. I will implement gradually the rasterization of labels and the "circularization" of polygons and labels, then we can test what works best.
|
||
transform = Scale([1.0 / specs["pixel_size"], 1.0 / specs["pixel_size"]], axes=("x", "y")) | ||
# points = PointsModel.parse(coords=arr, annotations=annotations, transformations={"global": transform}) | ||
points = PointsModel.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.
Here we get
INFO Instance key `cell_id` could be of type `pd.Categorical`. Consider
casting it.
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 warning would keep for now and then maybe remove
I made some changes in |
In my edits I in particular added the coordinate systems to |
ok great, gonna check that all files are consistent mirroring spatialdata sandbox and then will merge. |
one thing is that I added will merge this even without re-review hope it's fine |
Co-authored-by: Luca Marconato <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
should close respective PRs once this is merged