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

Import methods #746

Closed
wants to merge 18 commits into from
Closed

Import methods #746

wants to merge 18 commits into from

Conversation

philouail
Copy link
Collaborator

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<-).

@philouail philouail requested a review from jorainer May 24, 2024 10:45
@jorainer
Copy link
Collaborator

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
Copy link
Collaborator Author

philouail commented Jun 5, 2024

@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
Copy link
Collaborator

jorainer commented Jun 5, 2024

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

jorainer and others added 2 commits June 5, 2024 17:16
- Small fixes and formatting changes in the code.
- Expand unit tests to cover additional scenarios.
- Update/restructure documentation.
refactor: updates of documentation and extension of unit tests
@jorainer
Copy link
Collaborator

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
Copy link
Collaborator

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.

@philouail
Copy link
Collaborator Author

Closing because the code will be moved to a separate package. See MsIO.

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.

2 participants