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

msnset se coercions #284

Merged
merged 5 commits into from Dec 17, 2017

Conversation

Projects
None yet
3 participants
@arnesmits
Contributor

arnesmits commented Dec 8, 2017

First step towards an MSnSet to SE coercions.

@sgibb

Thanks for your PR and the great idea to convert an MSnSet into an SummarizedExperiment object and vice versa. Instead of a se2msnset and msnset2se function we would prefer a as(msnset, "SummarizedExperiment") and as(se, "MSnSet") method. You could use setAs("MSnSet", "SummarizedExpirment", function(from) {}) for that.

Additionally, I added some comments to your code (mostly about the coding style).

Show outdated Hide outdated R/msnset2se.R Outdated
Show outdated Hide outdated R/msnset2se.R Outdated
Show outdated Hide outdated R/msnset2se.R Outdated
Show outdated Hide outdated R/msnset2se.R Outdated
Show outdated Hide outdated R/msnset2se.R Outdated
Show outdated Hide outdated R/msnset2se.R Outdated
Show outdated Hide outdated R/msnset2se.R Outdated
Show outdated Hide outdated R/msnset2se.R Outdated
Show outdated Hide outdated R/msnset2se.R Outdated
Show outdated Hide outdated R/msnset2se.R Outdated
@arnesmits

This comment has been minimized.

Show comment
Hide comment
@arnesmits

arnesmits Dec 8, 2017

Contributor

I incorporated the style changes and comments. I kept se2msnset and msnset2se as internal functions, which I call within the setAs functions. Let me know if I overlooked something and/or there are other issues.

Contributor

arnesmits commented Dec 8, 2017

I incorporated the style changes and comments. I kept se2msnset and msnset2se as internal functions, which I call within the setAs functions. Let me know if I overlooked something and/or there are other issues.

@sgibb

Right direction but I have still some (code styling) comments.

Show outdated Hide outdated R/msnset2se.R Outdated
Show outdated Hide outdated R/msnset2se.R Outdated
Show outdated Hide outdated R/msnset2se.R Outdated
Show outdated Hide outdated R/msnset2se.R Outdated
Show outdated Hide outdated tests/testthat/test_msnset_se_coercions.R Outdated
Show outdated Hide outdated tests/testthat/test_msnset_se_coercions.R Outdated
@lgatto

This comment has been minimized.

Show comment
Hide comment
@lgatto

lgatto Dec 8, 2017

Owner

Also, @arnesmits, please add yourself as a contributor in the DESCRIPTION file.

Owner

lgatto commented Dec 8, 2017

Also, @arnesmits, please add yourself as a contributor in the DESCRIPTION file.

arnesmits and others added some commits Dec 11, 2017

Update DESCRIPTION
more SummarizedExperiment from suggests to imports
@sgibb

This comment has been minimized.

Show comment
Hide comment
@sgibb

sgibb Dec 12, 2017

Collaborator

@arnesmits: Sorry I was on the wrong track. If we just want to "Suggests" SummarizedExperiment you have to use SummarizedExperiment::assay after requireNamespace("SummarizedExperiment) (so you were right). Adding it to NAMESPACE would require to "import" or "depend" on it.

@lgatto + @arnesmits : Sry, I wasn't aware that we have to import SummarizedExperiment to provide an as method (otherwise we get a warning: in method for ‘coerce’ with signature ‘"SummarizedExperiment","MSnSet"’: no definition for class “SummarizedExperiment”). So requireNamespace is not necessary at all.

I am a little bit depressed that we have to add a dependency just for an as.

Collaborator

sgibb commented Dec 12, 2017

@arnesmits: Sorry I was on the wrong track. If we just want to "Suggests" SummarizedExperiment you have to use SummarizedExperiment::assay after requireNamespace("SummarizedExperiment) (so you were right). Adding it to NAMESPACE would require to "import" or "depend" on it.

@lgatto + @arnesmits : Sry, I wasn't aware that we have to import SummarizedExperiment to provide an as method (otherwise we get a warning: in method for ‘coerce’ with signature ‘"SummarizedExperiment","MSnSet"’: no definition for class “SummarizedExperiment”). So requireNamespace is not necessary at all.

I am a little bit depressed that we have to add a dependency just for an as.

@lgatto

This comment has been minimized.

Show comment
Hide comment
@lgatto

lgatto Dec 13, 2017

Owner

@sgibb - I moved that package to Imports to fix the error, but I'm happy to leave it Suggests to avoid the hard dependency.

Owner

lgatto commented Dec 13, 2017

@sgibb - I moved that package to Imports to fix the error, but I'm happy to leave it Suggests to avoid the hard dependency.

@sgibb

This comment has been minimized.

Show comment
Hide comment
@sgibb

sgibb Dec 13, 2017

Collaborator

Yes, but I am afraid that is not possible. I don't know if there is a way to provide a method for a class without importing this class.

Collaborator

sgibb commented Dec 13, 2017

Yes, but I am afraid that is not possible. I don't know if there is a way to provide a method for a class without importing this class.

@sgibb

This comment has been minimized.

Show comment
Hide comment
@sgibb

sgibb Dec 13, 2017

Collaborator

BTW: it is not a warning but a message and I already get the message:
in method for ‘coerce’ with signature ‘"IBSpectra","MSnSet"’: no definition for class “IBSpectra”
So we could move SummarizedExperiment to Suggests: and add a few more messages about missing class definitions to the MSnbase startup ... 😞

Collaborator

sgibb commented Dec 13, 2017

BTW: it is not a warning but a message and I already get the message:
in method for ‘coerce’ with signature ‘"IBSpectra","MSnSet"’: no definition for class “IBSpectra”
So we could move SummarizedExperiment to Suggests: and add a few more messages about missing class definitions to the MSnbase startup ... 😞

@lgatto

This comment has been minimized.

Show comment
Hide comment
@lgatto

lgatto Dec 13, 2017

Owner

Yes, I can live with the message.

Owner

lgatto commented Dec 13, 2017

Yes, I can live with the message.

@arnesmits

This comment has been minimized.

Show comment
Hide comment
@arnesmits

arnesmits Dec 14, 2017

Contributor

I agree that avoiding a hard dependency is preferred, so changed this back.

Contributor

arnesmits commented Dec 14, 2017

I agree that avoiding a hard dependency is preferred, so changed this back.

@sgibb

This comment has been minimized.

Show comment
Hide comment
@sgibb

sgibb Dec 15, 2017

Collaborator

@arnesmits After moving SummarizedExperiments from Imports to Suggests you also have to remove the entries from NAMESPACE. Could you please add some information to the MSnSet-class manual page.

Collaborator

sgibb commented Dec 15, 2017

@arnesmits After moving SummarizedExperiments from Imports to Suggests you also have to remove the entries from NAMESPACE. Could you please add some information to the MSnSet-class manual page.

@lgatto lgatto merged commit 9fe34ff into lgatto:master Dec 17, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
@lgatto

This comment has been minimized.

Show comment
Hide comment
@lgatto

lgatto Dec 17, 2017

Owner

Done now. See ./R/coerce.R and ./man/MSnSet-class.Rd and here for details. The unit tests are in `test_coerce.R.

Owner

lgatto commented Dec 17, 2017

Done now. See ./R/coerce.R and ./man/MSnSet-class.Rd and here for details. The unit tests are in `test_coerce.R.

@arnesmits

This comment has been minimized.

Show comment
Hide comment
@arnesmits

arnesmits Dec 19, 2017

Contributor

@lgatto + @sgibb: Thanks a lot for all your work!

Contributor

arnesmits commented Dec 19, 2017

@lgatto + @sgibb: Thanks a lot for all your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment