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

refactor: dump all data export functionality from xcms #757

Merged
merged 3 commits into from
Jul 26, 2024
Merged

Conversation

jorainer
Copy link
Collaborator

  • Remove the data export functionality (saveResults()) from xcms as it will be implemented (in a more structured and cleaner way) in the new MsIO package.

Advantage:

  • development can be done independently of xcms and Bioconductor release cycles.
  • have export/import methods not only for the xcms objects, but all other related ones (Spectra, MsBackend, MsExperiment, ...).
  • less package dependencies in xcms

Disadvantage:

  • I removed the exported methods and classes from xcms - but I don't think anybody was already using them.

- Remove the data export functionality (`saveResults()`) from *xcms* as it will
  be implemented (in a more structured and cleaner way) in the new *MsIO*
  package.
@sneumann
Copy link
Owner

  • I am not sold on the independence from BioC releases, if MsIO one day goes Bioc ...
  • the dependency bloat will then be in MsIO which then depends on a slew of packages like xcms, Spectra, ... But I don't see a way out of that. An advantage could be that we can combine e.g. xcms + Spectra into mzTab.

@jorainer
Copy link
Collaborator Author

I am not sold on the independence from BioC releases, if MsIO one day goes Bioc ...

sorry, was maybe not clear there. This is just for the short-term of course. We're currently experimenting and evaluating different export/import ways, so I just think we don't have a final version implemented until September (for the BioC release). Thus, it would be for now safer to use the MsIO package as a playground and don't risk having unfinished code in xcms (for the release).

Also, import/export of xcms result objects would make sense in xcms, true, but the xcms objects extend MsExperiment, Spectra etc objects. so, having the import/export of these objects in xcms is not good. and splitting them across the respective packages is also not ideal, especially because some code is shared (e.g. the "Param" classes and the import/export methods). Thus, we bundle them at the moment in the MsIO package - if it's settled we might decide to distribute the final functionality into the respective packages - but it has also advantages (mostly for maintainence) to keep all in one place.

For the dependency bloat - for now we have MsExperiment, Spectra, xcms "only" in Suggests, but not in imports. So, MsIO is quite "lean" to install. To import/export e.g. a xcms result object it is anyway required to (install and) load the xcms package FIRST, so, we don't have (at least as it looks now) any issues.

So, for now we're re-implementing the "plain text file" import/export in MsIO - in a cleaner way that we did in xcms (which e.g. supported only Spectra with an MsBackendMzR backend). Then, we're also playing with adding support for alabaster, Bioconductor's new way to save R objects in a language agnostic format.

So, to summarize: for now I would like to remove the old import/export code from xcms and have it implemented in a cleaner way in MsIO. Maybe later (for the next BioC release) we can bring it back into xcms - but I also see an advantage in having that functionality separately for ease of maintenance and bundling of similar functionality within a single package.

Copy link
Owner

@sneumann sneumann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, looks good to me. R CMD check also works locally. Yours, Steffen

@sneumann sneumann merged commit ea3f776 into devel Jul 26, 2024
1 of 3 checks passed
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

Successfully merging this pull request may close these issues.

3 participants