Suggested updates and fixes for PlainTextParam #7
Conversation
Addition of PlainTextParam method for Spectra, MsExperiment, XcmsExperiment
| @@ -37,24 +37,19 @@ setMethod("readMsObject", | |||
| #' @noRd | |||
| .store_xcmsexperiment <- function(x, path = tempdir()) { | |||
| .export_process_history(x, path = path) | |||
| if (xcms::hasChromPeaks(x)) | |||
| .export_chrom_peaks(x, path) | |||
| ## if (xcms::hasChromPeaks(x)) # maybe also export chromPeaks. | |||
There was a problem hiding this comment.
i'm confused aha. What do you mean maybe ? and the function after is not hashed ? We don't need to do that check ?
There was a problem hiding this comment.
true - I completely forgot that message. My thought was more what if an user exports an XcmsExperiment object without chrom peak results. It would maybe still be nice to be able to import such an "empty" object? so, chromPeaks()should return ALWAYS amatrix` with column names (but eventually no rows), so we could export/import that.
I would just like to avoid throwing an error if the XcmsExperiment is empty. I don't like errors.
There was a problem hiding this comment.
I added now unit tests checking export/import of empty objects. and along that I had to fix the export of MsExperiment: if @sampleData is an empty data frame I'm replacing it with a zero-row data frame with one column (sample_name). Otherwise the read.table call will fail.
| @@ -19,62 +19,73 @@ setMethod("saveMsObject", | |||
| recursive = TRUE, | |||
| showWarnings = FALSE) | |||
| ## sample data | |||
| write.table(as.data.frame(object@sampleData), sep = "\t", | |||
| sdata <- object@sampleData | |||
| if (!length(sdata)) # initialize with empty data frame | |||
There was a problem hiding this comment.
that's a workaround to ensure that a valid data frame is exported. If data.frame is completely empty, the content of the text file will be just "" and the read.table() call fails. With a single column, but no rows the data can be exported and imported.
|
@philouail , I've addressed some of your comments and also added support for empty object export/import (and added unit tests for that). |
| @@ -17,8 +17,10 @@ importFrom(jsonlite,write_json) | |||
| importFrom(methods,as) | |||
| importFrom(methods,callNextMethod) | |||
| importFrom(methods,existsMethod) | |||
| importFrom(methods,getFunction) | |||
There was a problem hiding this comment.
I think this can be removed ?
Major changes in this PR:
MsBackendMzRbackend,ms_experiment_sample_data.txtfor thesampleData()of anMsExperimentetc. This should make it a bit clearer which files belong to each other.spectraDataPathis now only a parameter forloadMsObject,MsBackendMzR- with the other objects just having...in their definition. This parameter might not be needed for other backends, or they might need even different/more parameters. Using...instead of a dedicated parameter gives us more flexibility in future.