-
Notifications
You must be signed in to change notification settings - Fork 28
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
Add spatial coordinates to Anndata when converting from SpatialExperiment #138
base: devel
Are you sure you want to change the base?
Conversation
Thanks! I will probably suggest some tweaks to the code when I have a chance to properly look at it but for now it would be great to add a couple of tests. Probably by finding a test |
Great, I've added some tests using the example Visium data used by |
@mcmero I have pushed some commits with some adjustments, I thought that would be quicker than giving you comments. I hope that is ok. Could you please check you are still happy with everything (and maybe test it with any objects you have)? Also, please add yourself as a contributor to the |
If coords has colnames, this causes isses with rd$set_axis(colnames(sce))
Thanks @lazappi, looks good! I've tested with some of my data and found that the column names on the
|
* origin/devel: Update when GHA actions are run Update GitHub Actions bot user config Move set git credentials step in pkgdown job
* origin/devel: Fix handling of missing rowData/colData
R/write.R
Outdated
# If converting SpatialExperiment object, add spatial coords to reducedDims | ||
if (inherits(sce, "SpatialExperiment")) { | ||
coords <- SpatialExperiment::spatialCoords(sce) | ||
if (ncol(coords) > 1) { | ||
colnames(coords) <- NULL | ||
SingleCellExperiment::reducedDim(sce, "spatial") <- coords | ||
} | ||
} |
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've just realised this should probably be moved to SCE2AnnData()
so it works if someone calls that directly. The documentation can probably also do there.
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.
Sure, I've pushed a commit with this change.
I think we would prefer to avoid losing the column names if possible. It would be helpful to now what the colnames of the spatial coords, what the colnames of the object are and the class of the spatial coords. Also, I fixed a bug that might affect this so if you could check again that would be good. Thanks! |
Makes sense. The error I was getting with my |
I added a test for coords without names which seems to work. The conversion to a If you are happy with this now I think it can be merged. |
…bject
That sounds good. With the colnames I meant example(read10xVisium, echo = FALSE)
colnames(spe) <- NULL
rownames(spatialCoords(spe)) <- NULL
writeH5AD(spe, "test.H5AD") Throws the error I posted above. It might be helpful to improve this error message, so I've added an error and test in the latest commit. It may be out of scope for this PR however so feel free to revert. |
* origin/devel: Fix GHA badge in README.md
Ok, that ended up being trickier than I expected but the issue with missing names should be fixed. If the checks pass I think we should be about done. |
The aim is to add the spatial coordinates to
adata.obsm["spatial"]
when converting SpatialExperiment objects viazellkonverter::writeH5AD
. Please see this issue for more context.@lazappi, you mentioned moving this to the R side and leaving the conversion code, so I've done this by adding the coordinates to
reducedDims
as a workaround.