search

sneumann / xcms

This is the git repository matching the Bioconductor package xcms: LC/MS and GC/MS Data Analysis
Other
177 stars 81 forks source link

Addition of PlainTextParam to the storeResults generic function #699

Closed philouail closed 7 months ago

philouail commented 7 months ago

This PR is for the addition of a method to save XcmsExperiment and MsExperiment objects as a collection of plaint text files.

The description of the Param details explains the name of the text files saved and what they contain/what function was operated to get the contents

@jorainer , here it is with the small changes that we talked about. One thing, I am not sure what action is added to the processing queue exactly and it's blocking me to do one the of UT (see my comments there). Also I need and XcmsExperiment object with features definitions, chrompeaks data, features definitions and all for the UT(you'll see it failed for the signature with XcmsExperiment). There is none in the testthat.R should I create one or there is something else I can use ?

jorainer commented 7 months ago

I would suggest to move the unit tests you have now defined into the tests/test_PlainTextParam.R file (then all the code in the PlainTextParam.R would have the unit tests in that file).

To get a XcmsExperiment object for your unit test you can use the loadXcmsData function to load one of the bundled (full) result objects. Maybe put something like this at the beginning of your unit test file:

xmse_full <- loadXcmsData("xmse")

I would avoid to use the variable xmse again, because it could then overwrite the one created in testthat.R and some unit tests might mysteriously fail afterwards.

That XcmsExperiment has all chrom peaks, adjusted retention times and correspondence results, so you can then check for presence of all files. To check the storeResults,MsExperiment,PlainTextParam method you can still use the mse variable, as that one is created in testthat.R.

philouail commented 7 months ago

just to be sure the test_PlainTestPram.R file should be in the testthat/ folder right ?

jorainer commented 7 months ago

just to be sure the test_PlainTestPram.R file should be in the testthat/ folder right ?

Yes, exactly.

jorainer commented 7 months ago

@sneumann can you please also check this PR? If OK, please merge and push to Bioconductor.

sneumann commented 7 months ago

Hi, PR looks good to me. So currently this is write-Only, we don't have any consumer of the JSON, and don't use the read_json() to read back. Is that correct ? Is this next on the list ? Yours, Steffen

jorainer commented 7 months ago

Hi Steffen @sneumann , yes, this is currently only write-only. We wanted to first have export functionality (including mzTab-M which will come in a separate package) and once we're OK with it add import/read functions. And yes, we re-import using read_json.