LieberInstitute / spatialLIBD

Code for the spatialLIBD R/Bioconductor package and shiny app
http://LieberInstitute.github.io/spatialLIBD/
78 stars 16 forks source link

Update document of `sampleid` argument in viz_clus #42

Closed boyiguo1 closed 1 year ago

boyiguo1 commented 1 year ago

There seems no sample_name variable in colData(spe) now. I wonder if the document should be updated accordingly too.

https://github.com/LieberInstitute/spatialLIBD/blob/1aecde89a664de02813352a31f71635625e998a5/R/vis_clus.R#L8-L9

Meanwhile, I wonder if the purpose of spatialLIBD is to be used as a viz tool for any spe object, or only spatialLIBD data. If it is for the prior case, I argue the sampleid argument with no default seems a little bit restrictive and reduces the generalizability of the function. Maybe consider making sampleid default to null such that it plots all spots in spe when no sample_id in the spe

lcolladotor commented 1 year ago

I won't make the default NULL for plotting all samples. That's what the vis_clus_grid() function is for http://research.libd.org/spatialLIBD/reference/vis_grid_clus.html. Though I could make the default unique(spe$sample_id)[1] in vis_clus() to make it consistent with the default of vis_clus_grid().

I'll also remove the colData(spe)$sample_name part, since as you noted, that's outdated documentation.

lcolladotor commented 1 year ago

For historical purposes, we used sample_name before SpatialExperiment existed and we hacked a bit SingleCellExperiment to contain the HumanPilot data. Once SpatialExperiment was made, then @bpardo99 wrote a function to update the SCE to an SPE object for the HumanPilot data, and that's when we switched to sample_id. See https://github.com/LieberInstitute/spatialLIBD/blob/e014bd8bd1c1b25864c6951326147f8f5db1b7d2/R/sce_to_spe.R#L66 for more.