msnset se coercions #284

arnesmits · 2017-12-08T08:24:10Z

First step towards an MSnSet to SE coercions.

sgibb

Thanks for your PR and the great idea to convert an MSnSet into an SummarizedExperiment object and vice versa. Instead of a se2msnset and msnset2se function we would prefer a as(msnset, "SummarizedExperiment") and as(se, "MSnSet") method. You could use setAs("MSnSet", "SummarizedExpirment", function(from) {}) for that.

Additionally, I added some comments to your code (mostly about the coding style).

sgibb · 2017-12-08T09:22:15Z

R/msnset2se.R

+#' @export
+se2msnset <- function(se) {
+  # Check input
+  assertthat::assert_that(inherits(se, "SummarizedExperiment"))


Currently we don't use assertthat for evaluation of input variables. While it is very convenient it don't think we are accepting a new dependency for just an easy if (inherits(se, "SummarizedExperiment")) check. BTW: assertthat is not listed in the DESCRIPTION file.

sgibb · 2017-12-08T09:28:51Z

R/msnset2se.R

+  assertthat::assert_that(inherits(se, "SummarizedExperiment"))
+
+  # Extract expression, feature and pheno data
+  raw <- SummarizedExperiment::assay(se)


Currently we also have no dependency on SummarizedExperiment. That means you have to add SummarizedExperiment to the DESCRIPTION file. I would suggest to add it into Suggests: and use if (requireNamespace("SummarizedExperiment")) here to check whether it is available or not.

The :: should not necessary. You only need it if you have two functions with the same name in two imported packages. Beside adding SummarizedExperiment to the DESCRIPTION file you have to import the class and the methods you use via NAMESPACE: importClassesFrom(SummarizedExperiment, SummarizedExperiment) and importFrom(SummarizedExperiment, assay, colData, rowData).

sgibb · 2017-12-08T09:35:22Z

R/msnset2se.R

+  # Extract expression, feature and pheno data
+  raw <- SummarizedExperiment::assay(se)
+  feat_data <- data.frame(SummarizedExperiment::rowData(se))
+  rownames(feat_data) <- se@NAMES


You should not access slots directly. Why not use names(se) here? I am not very familiar with SummarizedExperiment yet but are you sure that rownames(rowData(se)) is never set?

sgibb · 2017-12-08T09:38:30Z

R/msnset2se.R

+  pheno_data <- data.frame(SummarizedExperiment::colData(se))
+
+  # Generate MSnSet
+  msnset <- MSnbase::MSnSet(exprs = as.matrix(raw),


There is no need to use MSnbase:: because there are no other MSnSets around and the namespace of the package is always used first.

sgibb · 2017-12-08T09:39:12Z

R/msnset2se.R

+  # Generate MSnSet
+  msnset <- MSnbase::MSnSet(exprs = as.matrix(raw),
+                            pData = Biobase::AnnotatedDataFrame(pheno_data),
+                            fData = Biobase::AnnotatedDataFrame(feat_data))


We don't need Biobase:: as well. We are depending on Biobase so AnnotatedDataFrame is available without ::.

sgibb · 2017-12-08T09:42:56Z

R/msnset2se.R

+#' @export
+msnset2se <- function(msnset) {
+  # Check input
+  assertthat::assert_that(inherits(msnset, "MSnSet"))


See my comment above regarding assertthat.

sgibb · 2017-12-08T09:43:20Z

R/msnset2se.R

+  # Extract assay, rowData and colData
+  raw <- Biobase::exprs(msnset)
+  row_data <- Biobase::fData(msnset)
+  col_data <- Biobase::pData(msnset)


We don't need Biobase:: here as well.

sgibb · 2017-12-08T09:43:42Z

R/msnset2se.R

+  se <- SummarizedExperiment::SummarizedExperiment(assays = as.matrix(raw),
+                                                   rowData = row_data,
+                                                   colData = col_data)
+  return(se)


return not needed.

sgibb · 2017-12-08T09:44:18Z

R/msnset2se.R

+  col_data <- Biobase::pData(msnset)
+
+  # Generate SE
+  se <- SummarizedExperiment::SummarizedExperiment(assays = as.matrix(raw),


If SummarizedExperiment was attached by requireNamespace (which is missing here) the :: operator isn't needed.

I added requireNamespace, however I still need the :: operator.

sgibb · 2017-12-08T09:46:12Z

R/msnset2se.R

+
+  # Extract expression, feature and pheno data
+  raw <- SummarizedExperiment::assay(se)
+  feat_data <- data.frame(SummarizedExperiment::rowData(se))


Please don't use snake_case. We try to follow the bioc coding style and use camelCase

arnesmits · 2017-12-08T10:41:07Z

I incorporated the style changes and comments. I kept se2msnset and msnset2se as internal functions, which I call within the setAs functions. Let me know if I overlooked something and/or there are other issues.

sgibb

Right direction but I have still some (code styling) comments.

sgibb · 2017-12-08T19:01:26Z

R/msnset2se.R

+  stopifnot(inherits(se, "SummarizedExperiment"))
+
+	if(!requireNamespace("SummarizedExperiment")) {
+		stop("The SummarizedExperiment package is required for SummarizedExperiment to MSnSet conversion.")


Sorry for being petty but could you please use just 4 spaces for indentation (you used ~8) and keep lines shorter than 80 characters?

BTW: if you use setAs instead of an internal function the stopifnot(inherits(se, "SummarizedExperiment")) is superfluous.

sgibb · 2017-12-08T19:06:36Z

R/msnset2se.R

+  assertthat::assert_that(inherits(se, "SummarizedExperiment"))
+
+  # Extract expression, feature and pheno data
+  raw <- SummarizedExperiment::assay(se)


The :: should not necessary. You only need it if you have two functions with the same name in two imported packages. Beside adding SummarizedExperiment to the DESCRIPTION file you have to import the class and the methods you use via NAMESPACE: importClassesFrom(SummarizedExperiment, SummarizedExperiment) and importFrom(SummarizedExperiment, assay, colData, rowData).

sgibb · 2017-12-08T19:07:03Z

R/msnset2se.R

-                            fData = Biobase::AnnotatedDataFrame(feat_data))
-  return(msnset)
+  MSnSet(exprs = as.matrix(raw),
+  			 pData = AnnotatedDataFrame(phenoData),


Please fix the indentation here as well.

sgibb · 2017-12-08T19:08:33Z

R/msnset2se.R

+setAs("MSnSet", 
+			"SummarizedExperiment",
+			function(from) {
+				msnset2se(from)


The functions are very short and not used anywhere else. That's why I don't see the point of an additional internal function. Please put the function body into the setAs method.

@arnesmits - I believe you use use se2msnset and msnset2se in your package and possibly mention them in documentation or vignette. I think we can keep them, at least in a first instance, as they might already be used here and there. But the official as syntax should probably he preferred.

I am against exporting se2msnset and msnset2se:

We prefer the as syntax.

x2y is uncommon.

If we add it we would teach the user a "wrong" approach.

If a second package (MSnbase) supports them they become more common and seem more "correct".

To deprecate/defunct a function is effort (we have to remember it for more than one release cycle, documentation, ...) and we maybe won't ever do it.

@arnesmits - are you ok with marking the old forms as Defunct and suggest to use the standard as form, so we can redirect users that might already have used them to a more standard form?

I agree with both of you that the common as syntax would be preferable. The old form is mentioned in the documentation of my package, although it is not required in the default workflow. I will mark it as Defunct and suggest the as form, once this is incorporated in MSnbase.

sgibb · 2017-12-08T19:11:39Z

tests/testthat/test_msnset_se_coercions.R

@@ -21,6 +21,6 @@ test_that("se2msnset conversion", {
 	expect_equal(colnames(msnset_back), colnames(se))
 	expect_equal(rownames(msnset_back), rownames(se))
 	expect_equal(Biobase::exprs(msnset_back), SummarizedExperiment::assay(se))
-	expect_equal(Biobase::fData(msnset_back), data.frame(SummarizedExperiment::rowData(se), row.names = se@NAMES))
+	expect_equal(Biobase::fData(msnset_back), data.frame(SummarizedExperiment::rowData(se), row.names = names(se)))


I missed this one before. Why do you compare against msnset_back and not directly against msnset? And wouldn't be expect_equal(msnset, as(se, "MSnSet")) enough?

And the :: shouldn't be needed here.

One issue with the current coercion is that it only converts the assayData, phenoData and featureData. The other slots, such as qual, processingData, experimentData, protocolData and annotation are MSnSet specific. The SummarizedExperiment supports a simple list as metadata, so one solution could be to parse all MSnSet specific slots into a list which is stored in the SummarizedExperiment metadata and vice versa for SE to MSnSet conversion. The question is whether it is necessary to transfer this additional information and whether it is preferred to store this information in a simple list?

I will push the necessary as(x, "list") to MSnbase in the coming days, so that you can add these slots to the SummarizedExperiment's metadata slot. Will update this comment with more instructions when the code becomes available.

I have added the three as methods in the latest MSnbase on github. There is also MSnbase:::.reduce_list that will remove empty elements or those with and empty character ("") - MSnbase:::.reduce_list(as(processingData(x), "list")), so as to avoid adding missing data. The names of the elements could possibly be prefixed with MSnbase to make it explicit where these metadata originate from.

@lgatto: Thanks. In the end, I didn't use these functions as I could store the data.frame, MSnProcess, MIAPE and AnnotatedDataFrame objects in the metadata list of SummarizedExperiment.

The current implementation is a bit 'MSnSet-focused', because it will correctly coerce all MSnSet slots into SE and back to MSnSet. However, SE metadata that does not fit into one of the MSnSet slots will be lost in the SE to MSnSet coercion.

The issue with this is that once you have an object created with

se <- as(msnset, "SummarizedExperiment")

you will still need MSnbase to use se because of the MSnProcess and MIAPE objects in the metadata (AnnotatedDataFrame is defined in Biobase, which is a dependency of SummarizedExperiment). Having that assumption simplifies the code, but it limits the portability of that se object. @arnesmits - what is your typical use case?

I will work on this next week, and only transfer some information as a list to the SummarizedExperiment metadata slot, and document the behaviour in the manual page.

sgibb · 2017-12-08T19:12:00Z

tests/testthat/test_msnset_se_coercions.R

@@ -8,7 +8,7 @@ test_that("msnset2se conversion", {
 	expect_equal(colnames(msnset), colnames(se))
 	expect_equal(rownames(msnset), rownames(se))
 	expect_equal(Biobase::exprs(msnset), SummarizedExperiment::assay(se))
-	expect_equal(Biobase::fData(msnset), data.frame(SummarizedExperiment::rowData(se), row.names = se@NAMES))
+	expect_equal(Biobase::fData(msnset), data.frame(SummarizedExperiment::rowData(se), row.names = names(se)))


The :: shouldn't be needed here.

lgatto · 2017-12-08T19:56:42Z

Also, @arnesmits, please add yourself as a contributor in the DESCRIPTION file.

more SummarizedExperiment from suggests to imports

sgibb · 2017-12-12T19:12:43Z

@arnesmits: Sorry I was on the wrong track. If we just want to "Suggests" SummarizedExperiment you have to use SummarizedExperiment::assay after requireNamespace("SummarizedExperiment) (so you were right). Adding it to NAMESPACE would require to "import" or "depend" on it.

@lgatto + @arnesmits : Sry, I wasn't aware that we have to import SummarizedExperiment to provide an as method (otherwise we get a warning: in method for ‘coerce’ with signature ‘"SummarizedExperiment","MSnSet"’: no definition for class “SummarizedExperiment”). So requireNamespace is not necessary at all.

I am a little bit depressed that we have to add a dependency just for an as.

lgatto · 2017-12-13T13:39:03Z

@sgibb - I moved that package to Imports to fix the error, but I'm happy to leave it Suggests to avoid the hard dependency.

sgibb · 2017-12-13T19:50:28Z

Yes, but I am afraid that is not possible. I don't know if there is a way to provide a method for a class without importing this class.

sgibb · 2017-12-13T21:20:21Z

BTW: it is not a warning but a message and I already get the message:
in method for ‘coerce’ with signature ‘"IBSpectra","MSnSet"’: no definition for class “IBSpectra”
So we could move SummarizedExperiment to Suggests: and add a few more messages about missing class definitions to the MSnbase startup ... 😞

lgatto · 2017-12-13T22:06:20Z

Yes, I can live with the message.

arnesmits · 2017-12-14T08:55:57Z

I agree that avoiding a hard dependency is preferred, so changed this back.

sgibb · 2017-12-15T19:42:33Z

@arnesmits After moving SummarizedExperiments from Imports to Suggests you also have to remove the entries from NAMESPACE. Could you please add some information to the MSnSet-class manual page.

lgatto · 2017-12-17T20:07:32Z

Done now. See ./R/coerce.R and ./man/MSnSet-class.Rd and here for details. The unit tests are in `test_coerce.R.

arnesmits · 2017-12-19T09:26:02Z

@lgatto + @sgibb: Thanks a lot for all your work!

msnset se coercions

11294e0

sgibb requested changes Dec 8, 2017

View reviewed changes

changes regarding Sebastian's comments

6c60f19

sgibb requested changes Dec 8, 2017

View reviewed changes

arnesmits and others added 2 commits December 11, 2017 10:28

transfer to setAs

9467796

Update DESCRIPTION

b96dc91

more SummarizedExperiment from suggests to imports

MSnSet slots to SE metadata

9fe34ff

lgatto merged commit 9fe34ff into lgatto:master Dec 17, 2017

msnset se coercions #284

msnset se coercions #284

Conversation

arnesmits commented Dec 8, 2017

sgibb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnesmits commented Dec 8, 2017

sgibb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgatto commented Dec 8, 2017

sgibb commented Dec 12, 2017

lgatto commented Dec 13, 2017

sgibb commented Dec 13, 2017

sgibb commented Dec 13, 2017

lgatto commented Dec 13, 2017

arnesmits commented Dec 14, 2017

sgibb commented Dec 15, 2017

lgatto commented Dec 17, 2017

arnesmits commented Dec 19, 2017 • edited Loading

arnesmits commented Dec 19, 2017 •

edited

Loading