Skip to content
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

cbind: unique/duplicated colnamesare left asis/made unique; read10xVisium: keep barcodes as colData; spatialCoords/<-: withDimnames argument #128

Open
wants to merge 12 commits into
base: devel
Choose a base branch
from

Conversation

HelenaLC
Copy link
Collaborator

@HelenaLC HelenaLC commented Nov 17, 2022

Addressing #100 ...

  • cbind assures unique/duplicated colnamesare left asis/made unique
  • in case of duplicates, column names are prefixed with n_... where
    n corresponds to the number of the object being combined
  • to retain original barcode identities, these are now kept as colData
    by read10xVisium (previously used as row names and dropped)

... and #103

  • added argument withDimnames to spatialCoords getter & setter
  • discrepancies b/w colnames(x) and rownames(spatialCoords(x)) throw an error,
    unless withDimnames=FALSE or the latter are NULL

@HelenaLC
Copy link
Collaborator Author

So, can someone accept this if we're okay with the GA (& changes, ofc)?

@HelenaLC HelenaLC changed the title cbind: unique/duplicated colnamesare left asis/made unique; read10xVisium: keep barcodes as colData cbind: unique/duplicated colnamesare left asis/made unique; read10xVisium: keep barcodes as colData; spatialCoords/<-: withDimnames argument Nov 21, 2022
@drighelli
Copy link
Owner

drighelli commented Nov 22, 2022

the GHA fail during the tests:

── Error ('test_SpatialExperiment-spatialCoords.R:27'): spatialCoords<-,matrix ──
Error in `rownames(xyz) <- NULL`: object 'xyz' not found

do you know why?


test_that("spatialCoords<-,matrix", {
# no 'rownames(value)' passes
rownames(xyz) <- NULL
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are no xyz in the read10xVisium example, I suppose the GHA error comes from here...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Damn it, I had reversed the order of some lines... should be fixed now.

#' @importFrom SingleCellExperiment int_colData<-
#' @export
setReplaceMethod("spatialCoords",
c("SpatialExperiment", "matrix"),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a warning that I suppose is coming from the "matrix" instead of a "value"

Warning:     ‘spatialCoords<-’
    ‘\S4method{spatialCoords<-}{SpatialExperiment,NULL}’
    ‘\S4method{spatialCoords<-}{SpatialExperiment,matrix}’
  The argument of a replacement function which corresponds to the right
  hand side must be named ‘value’.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I saw. But I don’t understand because it’s good locally. I’m on it…

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed I don't get why, but have you tried something like: c("SpatialExperiment", "value") ?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's because the generic is defined as setGeneric("spatialCoords<-", function(x, value, withDimnames=TRUE) standardGeneric("spatialCoords<-"))

Copy link
Collaborator Author

@HelenaLC HelenaLC Nov 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

…the signature has to be classes, so “NULL” and “matrix” (not “value”) is correct. It’s saying the generic and methods don’t match. But I don’t see why not as they both have “value” in the function definition in the same order…

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I saw that, I'm looking over the internet, but still I'm not able to understand the motivation for this warning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants