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

Move import/export methods for MsBackendMzR from xcms #2

Closed
jorainer opened this issue Jun 12, 2024 · 8 comments
Closed

Move import/export methods for MsBackendMzR from xcms #2

jorainer opened this issue Jun 12, 2024 · 8 comments

Comments

@jorainer
Copy link
Member

Move the functionality to serialize a MsBackendMzR object to a text file from xcms (in this PR).

  • add the param class/function. Maybe renaming from PlainTextParam to FlatFileParam?
  • add the import/export methods.

The key concept/question will be if we can add S4 methods for e.g. the MsBackendMzR class without importing the Spectra package.

@jorainer
Copy link
Member Author

@philouail , can you please move the code over from xcms? I would put the parameter object definition and functions into a PlainTextParam.R file (or FlatFileParam.R file?) and then add a method for MsBackendMzR in a MsBackendMzR.R file. Let's start with that and then successively add methods for Spectra, MsExperiment etc on top (all in their .R files). For the documentation, that will be a bit tricky maybe how to put that, but I think it would make sense to have for each class (e.g. MsBackendMzR a documentation, and in that put the individual ones for the parameters (e.g. saveMsObject,MsBackendMzR,PlainTextParam etc ).

We can obviously think of an other way to structure the files and documentation.

@philouail
Copy link
Collaborator

philouail commented Jun 24, 2024

  • regarding the naming, I would stay with PlainTextParam , Json is a type of plain text file right ? so it makes more sense than using the word 'flat' no ?
  • I will move the code !
  • in term of sharing it with people, did you have a plan ? I would like to wait until everything is ready: such as other basic MsBackend implementation, maybe the mzTab-m export and import, summarized experiment ?

@philouail
Copy link
Collaborator

philouail commented Jun 24, 2024

Little addition in term of to-do steps ( and after discussing with @lgatto) after moving everything, we should discuss what can be changed as to use the alabaster package. Here i'm talking about the PlainTextParam code ( or then it could be renamed somehow if we only use Json export maybe we should indicate that).
The package is sturdy and there are validation of object (we then don't need to worry too much about that). And we shouldn't try to reinvent what exist.
Also would motivate using this for the summarizedExperiment object import/export.

I will look a bit more into the package and test the functions ! :)

@philouail
Copy link
Collaborator

The key concept/question will be if we can add S4 methods for e.g. the MsBackendMzR class without importing the Spectra package.

Do you not want to be dependent on Spectra ?

@jorainer
Copy link
Member Author

Do you not want to be dependent on Spectra ?

no. ideally, this package should not depend on Spectra or MsExperiment et al. If a user wants to export or import e.g. a MsBackendMzR or similar object he/she will anyway have loaded the Spectra package before. so, no need to make this package dependent on the other.

@jorainer
Copy link
Member Author

Re alabaster: agree that we should not reinvent stuff. but we need also to ensure that the code provides functionality we want/need. I need to first have a close look at alabaster... then we discuss.

@philouail
Copy link
Collaborator

Do you not want to be dependent on Spectra ?

no. ideally, this package should not depend on Spectra or MsExperiment et al. If a user wants to export or import e.g. a MsBackendMzR or similar object he/she will anyway have loaded the Spectra package before. so, no need to make this package dependent on the other.

Mmmm okay so I think we need to discuss more because @lgatto saw it the other way around, with putting the MsIO package as "enhance" in e.g. the Spectra package, but MsIO by itself is dependent on these packages. (If I understood well).
I also am not sure how not to make it dependent on spectra as I am using functionalities of the Spectra package. So we can discuss that together next week :)

and yes alabaster on paper I believe it is very nice and basically do what we do by serializing (even though i haven't managed to make it work with our objects...)

@philouail
Copy link
Collaborator

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

No branches or pull requests

2 participants