satijalab / seurat-wrappers

Community-provided extensions to Seurat
GNU General Public License v3.0
299 stars 129 forks source link

Adapt SEraster for Seurat spatial objects #190

Open zskylarli opened 5 months ago

zskylarli commented 5 months ago

This PR implements SEraster, specifically functions rasterizeGeneExpression, rasterizeCellType, and permutateByRotation, for Seurat objects. Functions can take either one object or a list of objects, and return in the same format.

Changes:

Notes:

Resources:

dcollins15 commented 4 months ago

Another edge case to keep in mind is that not all FOV instances will have centroids and (correct me if I'm wrong) SEraster doesn't handle cell segmentations. Whenever we make a call to GetTissueCoordinates.FOV/GetTissueCoordinates.VisiumV2 we need to be specifying which = "centroids" (and check that this doesn't raise any errors).

What is the expected behavior in this case?

zskylarli commented 4 months ago

Another edge case to keep in mind is that not all FOV instances will have centroids and (correct me if I'm wrong) SEraster doesn't handle cell segmentations. Whenever we make a call to GetTissueCoordinates.FOV/GetTissueCoordinates.VisiumV2 we need to be specifying which = "centroids" (and check that this doesn't raise any errors).

What is the expected behavior in this case?

Will check with the main author but seems counterintuitive to rasterize segmentations.

Also, have tried integrating your fix, but it ends up being that there are too many differences between VisiumV2 instance vs a normal FOV that it is almost cleaner to just implement by class than it is to use a lot of different tryCatchs; for example, the lack of a segmentation for V2 becomes an issue with CreateFOV. Will continue to clean the code but I actually can't think of a better solution than just to handle different classes separately.

zskylarli commented 4 months ago

Okay, I think I understand what you mean now and have tried to implement the suggestions. One issue remaining is that I can't figure out how the radius is used for plotting FOV objects - I see that the Radius() function is called within GeomSpatial to get the spot.size for SpatialPlots, but I actually have no idea how this is handled for SingleImagePlots; assuming it's different since it's inheriting ggplot's geom_point instead of using our internal geom_spatial? Let me know if you have any ideas.

dcollins15 commented 4 months ago

Okay, I think I understand what you mean now and have tried to implement the suggestions.

At a quick glance createRasterizedObject is looking pretty slick now - nicely done.

One issue remaining is that I can't figure out how the radius is used for plotting FOV objects - I see that the Radius() function is called within GeomSpatial to get the spot.size for SpatialPlots, but I actually have no idea how this is handled for SingleImagePlots; assuming it's different since it's inheriting ggplot's geom_point instead of using our internal geom_spatial? Let me know if you have any ideas.

I think it may just be ignored? ImageFeaturePlot and ImageDimPlot both have a size parameter that appears to get handed down to geom_point inside SingleImagePlot. Are you running into plotting issues post-rasterization?

zskylarli commented 4 months ago

Yeah, just that the radius isn't really passed into the plotting functions, so adjusting size will scale the points correctly but adjusting the radius for spots in the rasterized object doesn't actually seem to do anything.

dcollins15 commented 4 months ago

noice

The vignette is easy to follow and everything seems to be working as expected 👌 I'm still in the process of writing out some suggestions to tidy the code up some but I've got a couple of high-level comments that are worth considering in the meantime:

  1. Namespace collisions - we probably don't want to have to make calls to SeuratWrappers:: in the vignette. We wouldn't have to if we didn't import the SERaster package or otherwise updated the SeuratWrapper methods to use PascalCase instead of camelCase. Since Seurat usually uses PascalCase for function names this might be kind of an elegant way to go.

  2. permutateByRotation is currently clobbering the image cell names with simple indices. This means that until rasterizeGeneExpression or rasterizeCellType is called, the Seurat instances are invalid and cannot be passed to any function that checks that validObject passes (e.g. NormalizeData). Furthermore, the fact that the clobbered cell names still work with the downstream methods might suggest that rasterizeGeneExpression and rasterizeCellType will not work as expected if handed an object where the counts index differs from the coordinates index (e.g. the image has been previously cropped).

  3. I think it might be more ergonomic to have permutateByRotation return a single Seurat instance but with multiple FOVs. Then, instead of allowing a list to be passed to rasterizeGeneExpression and rasterizeCellType we just have to support multiple image names to be specified. This isn't possible to do with a SpatialExperiment so this would be a small departure from the underlying API, but I think this is a better way to capture the workflow with our data model.

Once I've got my final code suggestions pushed up we can touch base and settle on action items to try to get this merged in as soon as practical 👍