Import methods - Githubissues

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

Import methods #746

Open philouail opened 1 month ago

philouail commented 1 month ago

Here are the implementation of the generics function loadResults() and its respective method for the parameters RDataParam and PlainTextParam.

I still have bugs in order to build examples/unit test with the spectra files path, I need some help regarding that.

PS: the github checks crash because of the new Spectra functions i added to the namespace (e.g dataStorageBasePath, dataStorageBasePath<-).

jorainer commented 1 month ago

The code is great, but I just think we need to re-evaluate the whole text import/export thing and design the methods and the whole process better - so that it is future proof and we can support other Spectra backends etc later.

philouail commented 3 weeks ago

@jorainer so here is the updated code with method to store and load MsBackendMzR and Spectra objects.

The checks passes ! It seems much more sturdy, thanks a lot for the helps to re-structure.

I wanted you to check what it looks like now but there are still some things that needs to be discussed:

Should i implement other backends in this same PR ? MsBackendDataFrame, MsBackendMemore and MsBackendSQlite pops to mind and shouldn't be super complicated to implement
Should MsBackend and Spectra methods be already moved to the Spectra package ?
How to make unit test for processHistory ?
How to make unit test for the spectraPath parameter ?

(if you see that i changed random .R files, i just fixed some dependencies)

jorainer commented 3 weeks ago

thanks @philouail ! I'll check/evaluate your branch locally and eventually make some changes/requests to that.

jorainer commented 2 weeks ago

The only thing we might need to check before making this an official PR is to re-evaluate the names of the methods - if we want to change them it would be better to already start fixing them here.

jorainer commented 2 weeks ago

After a dev call with Laurent - we agreed that it might eventually be better to implement and test the import/export functionality in a separate package MsIO.