Bioconductor / Contributions

Contribute Packages to Bioconductor
135 stars 33 forks source link

rgoslin submission #2485

Closed nilshoffmann closed 2 years ago

nilshoffmann commented 2 years ago

Update the following URL to point to the GitHub repository of the package you wish to submit to Bioconductor

Confirm the following by editing each check box to '[x]'

I am familiar with the essential aspects of Bioconductor software management, including:

For questions/help about the submission process, including questions about the output of the automatic reports generated by the SPB (Single Package Builder), please use the #package-submission channel of our Community Slack. Follow the link on the home page of the Bioconductor website to sign up.

bioc-issue-bot commented 2 years ago

Hi @nilshoffmann

Thanks for submitting your package. We are taking a quick look at it and you will hear back from us soon.

The DESCRIPTION file for this package is:

Package: rgoslin
Type: Package
Title: Lipid Shorthand Name Parsing and Normalization
Version: 0.99.1
Date: 2021-12-10
Authors@R: c(
    person("Nils", "Hoffmann", email = "nils.hoffmann@cebitec.uni-bielefeld.de", role=c("aut","cre"), comment = c(ORCID = "0000-0002-6540-6875")),
    person("Dominik", "Kopczynski", email = "dominik.kopczynski@univie.ac.at", role=c("aut"), comment = c(ORCID = "0000-0001-5885-4568")))
Description: The R implementation for the Grammar of Succint Lipid Nomenclature
    parses different short hand notation dialects for lipid names. It 
    normalizes them to a standard name. It further provides calculated 
    monoisotopic masses and sum formulas for each successfully parsed lipid name and supplements it with LIPID MAPS
    Category and Class information. Also, the structural level and further structural
    details about the head group, fatty acyls and functional groups are returned, where applicable.
License: MIT + file LICENSE
Imports: Rcpp (>= 1.0.3)
LinkingTo: Rcpp, testthat
Suggests: 
    pkgKitten,
    testthat (>= 2.1.0),
    knitr,
    rmarkdown,
    kableExtra,
    dplyr,
    BiocManager
RoxygenNote: 7.1.2
Encoding: UTF-8
VignetteBuilder: knitr
biocViews: Software, Lipidomics, Metabolomics, Preprocessing, Normalization, MassSpectrometry
BugReports: https://github.com/lifs-tools/rgoslin/issues
URL: https://github.com/lifs-tools/rgoslin
bioc-issue-bot commented 2 years ago

A reviewer has been assigned to your package. Learn what to expect during the review process.

IMPORTANT: Please read this documentation for setting up remotes to push to git.bioconductor.org. It is required to push a version bump to git.bioconductor.org to trigger a new build.

Bioconductor utilized your github ssh-keys for git.bioconductor.org access. To manage keys and future access you may want to active your Bioconductor Git Credentials Account

bioc-issue-bot commented 2 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/rgoslin to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

nilshoffmann commented 2 years ago

@PeteHaitch I have updated my repository at https://github.com/lifs-tools/rgoslin, removing the Rstudio project file that seems to have caused the build error, according to the logs. I do not seem to have rights to push to Bioconductor's git at this point. How should we proceed?

lshep commented 2 years ago

@nilshoffmann you should have permission to push to git.bioconductor.org. You will need to activate your GitCredentials account as mentioned about and add ssh-keys for access. https://git.bioconductor.org/BiocCredentials . Then follow the instructions for pushing to git.bioconductor.org https://bioconductor.org/developers/how-to/git/new-package-workflow/

bioc-issue-bot commented 2 years ago

Received a valid push on git.bioconductor.org; starting a build for commit id: 921329133ff613d89623f4a0cd8a8c688fb3a7c8

bioc-issue-bot commented 2 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

Congratulations! The package built without errors or warnings on all platforms.

Please see the build report for more details. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/rgoslin to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

PeteHaitch commented 2 years ago

Thanks @nilshoffmann. I aim to do the initial review this week (mid-next week at the latest).

PeteHaitch commented 2 years ago

Hi @nilshoffmann,

I see that rgoslin provides R bindings to the 'Goslin grammars' and that there are analogous packages for other programming languages. I started to review rgoslin, but before I go too far I'd like to clarify why you chose to submit to Bioconductor rather than, say, CRAN? In asking this, I don't mean to discourage the submission of rgoslin to Bioconductor.

I appreciate that lipids could certainly fall under Bioconductor's broad aim of supporting the "analysis of data from current and emerging biological assays", but rgoslin currently does not depend on any Bioconductor packages nor do the examples demonstrate how rgoslin would fit into a workflow that makes use of existing Bioconductor software around lipidomics, metabolomics, etc.

Since a key aim of the Bioconductor project is package interoperatability, some demonstration of this is generally considered essential. Are there packages within the LipidOmics, Metabolomics, or MassSpectrometry biocViews terms that you could use in your vignette to demonstrate how rgoslin fits into a typical analysis to properly showcase its features and benefits to a user?

Cheers, Pete

nilshoffmann commented 2 years ago

Hi @PeteHaitch,

thanks for the review and feedback on rgoslin. Proper lipid naming and reporting is still an unsolved problem. Goslin and rgoslin for R, aim at simplifying the process of parsing different shorthand nomenclatures that are currently in active use and aims to standardize lipid naming by providing a normlized shorthand lipid name, following the latest recommendations of the lipidomics standards initiative. As such, it is also the first library to support this use case in R and, as you already checked, many other programming languages.

I am in contact or have been with the authors of lipidr @ahmohamed, xcms @sneumann, @michaelwitting and others. I will be happy to discuss different examples / use cases to include in the rgoslin vignette with them, or even direct inclusion of rgoslin in one of these libraries. Since rgoslin is a utility library I designed it to have minimal dependencies.

Cheers, Nils

PeteHaitch commented 2 years ago

Hi @nilshoffmann,

Thanks for the above response. Overall, the package is in good shape. For acceptance into Bioconductor, I have a few Required points, as well as some Recommended points, that I would ask you to first please address. I've also asked someone from the core Bioconductor team to chime in on a couple of points (tagging @vjcitn).

Given that you are managing a number of language-specific interfaces to goslin, if the Bioconductor-specific requirements seem too burdensome then you should consider submission to CRAN. As a utility library for biological data, I can see arguments for submission of rgoslin to either Bioconductor or CRAN. Bioconductor packages, such as lipidr and xcms, would still be able to depend on rgoslin if it was available on CRAN. Again, this is not intended to discourage submission to Bioconductor but I just wanted to make sure you aware of all the options :)

Required

if (!require("BiocManager", quietly = TRUE))
    install.packages("BiocManager")

BiocManager::install("rgoslin")

Please also add these to the README.md (replacing the current devtools::install_github()-based instructions).

Recommended

PeteHaitch commented 2 years ago

Consider removing pkgdown. Once the package is accepted into Bioconductor, it will have a software landing page and links to the documentation and vignettes available from https://bioconductor.org/packages/rgoslin/. I'll ask @vjcitn to chime in with any 'official' advice on whether separate pkgdown-based sites are okay.

I've checked with the BioC core team and there's no need to remove pkgdown.

You probably no longer need pkgKitten in Suggests in the DESCRIPTION. As far as I'm aware, this is only needed/used to setup the initial structure of an R package and it's then no longer required.

PeteHaitch commented 2 years ago

Hi @nilshoffmann ,

Do you wish to proceed with the submission? We like to see some activity on the issue within 3 weeks. It's no problem if it's taking longer, but for now I will close the issue and you can re-open it if you wish to proceed and when you are ready.

Cheers, Pete

nilshoffmann commented 2 years ago

Hi @PeteHaitch ,

thanks for the heads up. I will try to address the remaining issues until the end of this week.

Cheers, Nils

bioc-issue-bot commented 2 years ago

This issue is being closed because there has been no progress for an extended period of time. You may reopen the issue when you have the time to actively participate in the review / submission process. Please also keep in mind that a package accepted to Bioconductor requires a commitment on your part to ongoing maintenance.

Thank you for your interest in Bioconductor.

nilshoffmann commented 2 years ago

@PeteHaitch I am ready to recommence with the submission. How do I reopen the issue?

bioc-issue-bot commented 2 years ago

Dear @nilshoffmann ,

We have reopened the issue to continue the review process. Please remember to push a version bump to git.bioconductor.org to trigger a new build.

bioc-issue-bot commented 2 years ago

Received a valid push on git.bioconductor.org; starting a build for commit id: fee0a456877a07d062ce0c3edc615bf44c720080

bioc-issue-bot commented 2 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

Congratulations! The package built without errors or warnings on all platforms.

Please see the build report for more details. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/rgoslin to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

nilshoffmann commented 2 years ago

Current status of requested changes:

Required

if (!require("BiocManager", quietly = TRUE))
    install.packages("BiocManager")

BiocManager::install("rgoslin")

Recommended

Is it worth simplifying the R-level API to essentially parseLipidNames()? As far as I can tell, there is a one-to-one mapping between the length of lipidNames and the number of rows in the returned data.frame, so the 'non-pluralised' versions of the two main functions (parseLipidName() and parseLipidNameWithGrammar()) feel redundant.

Consider adding tests that cover the error cases.

PeteHaitch commented 2 years ago

Thanks, @nilshoffmann. Please post a message once you're done addressing the review comments and I'll take another look

bioc-issue-bot commented 2 years ago

Received a valid push on git.bioconductor.org; starting a build for commit id: 2b776de70ffdee34613d4d0e1054af43c82d6a4c

bioc-issue-bot commented 2 years ago

Received a valid push on git.bioconductor.org; starting a build for commit id: 2a90a6937f0d893b4a6ee52c2afdbbdbcfa66ac5

bioc-issue-bot commented 2 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

Congratulations! The package built without errors or warnings on all platforms.

Please see the build report for more details. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/rgoslin to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 2 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/rgoslin to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 2 years ago

Received a valid push on git.bioconductor.org; starting a build for commit id: 703e6222f8a1cc9709fda65818ff1e03dcb1f207

bioc-issue-bot commented 2 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/rgoslin to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 2 years ago

Received a valid push on git.bioconductor.org; starting a build for commit id: 62b02b2642d603eab399ca18ec2b08b902f72f04

bioc-issue-bot commented 2 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

Congratulations! The package built without errors or warnings on all platforms.

Please see the build report for more details. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/rgoslin to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 2 years ago

Received a valid push on git.bioconductor.org; starting a build for commit id: 90cfedcf5cebbdf88948db6d7dabae7a547bc6f8

bioc-issue-bot commented 2 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

Congratulations! The package built without errors or warnings on all platforms.

Please see the build report for more details. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/rgoslin to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

nilshoffmann commented 2 years ago

Current status of requested changes:

Required

* [x]  The vignette must demonstrate interoperability with other Bioconductor software. It sounds like some thought has been given to demonstrating use cases or examples, so if you add a section to the vignette that will suffice. (I would advise against directly including rgoslin in one of these other packages because I suspect it will complicate maintenance).

* [x]  The vignette must have an Introduction section (essentially the existing Prerequisites)

* [x]  The vignette must have an Installation instruction with the official Bioconductor installation instructions (see https://contributions.bioconductor.org/docs.html?q=installation#installation). Once the package is accepted into Bioconductor, these will look like
if (!require("BiocManager", quietly = TRUE))
    install.packages("BiocManager")

BiocManager::install("rgoslin")
* [x]  Please also add these to the README.md (replacing the current devtools::install_github()-based instructions).

* [x]  Please add a table of contents to the vignette

Recommended

Is it worth simplifying the R-level API to essentially parseLipidNames()? As far as I can tell, there is a one-to-one mapping between the length of lipidNames and the number of rows in the returned data.frame, so the 'non-pluralised' versions of the two main functions (parseLipidName() and parseLipidNameWithGrammar()) feel redundant.

* [x]  Could parseLipidNames(lipidNames) change to something like parseLipidNames(lipidNames, grammer = NULL) where grammar = NULL is equivalent to the current parseLipidNames(lipidNames) with automagic guessing of the grammar and parseLipidNames(lipidNames, grammer = value) is equivalent to the current parseLipidNamesWithGrammar(lipidNames, grammer = value)?
  If you want to retain parseLipidNames() and parseLipidNamesWithGrammar() as two separate functions, then if parseLipidNamesWithGrammar() is not supplied with a grammar (e.g., parseLipidNamesWithGrammar("PC 32:1")) then it should probably return an error to the effect of "Please supply a 'grammar'" rather than returning NULL.

* [x]  Consider adding a inst/CITATION file (see https://contributions.bioconductor.org/citation.html)

* [x]  What is renovate.json needed for? (_removed_)

* [x]  The BiocStyle package is encouraged for vignette formatting.
  (https://contributions.bioconductor.org/docs.html?q=vignette#installation)

* [x]  Vignette needs a table of contents

* [x]  Consider removing pkgdown. Once the package is accepted into Bioconductor, it will have a software landing page and links to the documentation and vignettes available from https://bioconductor.org/packages/rgoslin/. I'll ask @vjcitn to chime in with any 'official' advice on whether separate pkgdown-based sites are okay. (_removed_)

Consider adding tests that cover the error cases.

* [x]  E.g., a silly (perhaps unrealistic) example like parseLipidNames(c("A", "B")) and parseLipidNames(c(1, 2)) may not be triggering the intended error message?

* [x]  You can run covr::report() for a standalone code coverage report (this is how I saw that these lines are not covered by the current unit tests).

* [x]  There's a broken link in the README and vignette to the C# implementation (https://github.com/lifs-tools/csgoslin)

* [ ]  Again tagging @vjcitn, would a C-expert mind checking the src/Makevars?

@PeteHaitch I have added an example usage with lipidr in the vignette and have addressed all of your remarks. Only the external check of Makevars by @vjcitn is still open.

PeteHaitch commented 2 years ago

Thanks, @nilshoffmann! I'll take a look in the next few days (although might not be until next week)

PeteHaitch commented 2 years ago

Hi @nilshoffmann

Some additional comments based on the changes:

I implemented fixes for those ticked ones, which I planned to submit as a PR to https://github.com/lifs-tools/rgoslin. However, this github-hosted repo wasn't in-sync with the Bioconductor-hosted git repo so I couldn't do that. Instead, I've attached the output of git diff bioc-re-review > bioc-re-review-diff.txt, i.e. comparing the master branch of the Bioconductor-hosted git to my bioc-re-review branch. You may need to manually apply the changes I ticked off, but hopefully the patch helps somewhat.

Once those last issues are address I think we'll be ready to accept the package.

Thanks, Pete bioc-re-review-diff.txt

bioc-issue-bot commented 2 years ago

Received a valid push on git.bioconductor.org; starting a build for commit id: 0892bb4d6adeb3805b06b8643f83a552c5aea890

nilshoffmann commented 2 years ago

Hi @PeteHaitch thanks for the style fixes. Spaces versus tabs, I see :-).

As to why there is a difference between the GitHub repo and the one on Bioconductor: We have quite some large files included in the cppgoslin and goslin subtrees that would not pass the bioc-checks and would not need to be included for the R implementation to run. I am currently merging the contents manually between the two repos, but will switch to using proper patches due to your remark.

Thanks for this great and interactive review!

bioc-issue-bot commented 2 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

Congratulations! The package built without errors or warnings on all platforms.

Please see the build report for more details. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/rgoslin to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 2 years ago

Your package has been accepted. It will be added to the Bioconductor nightly builds.

Thank you for contributing to Bioconductor!

Reviewers for Bioconductor packages are volunteers from the Bioconductor community. If you are interested in becoming a Bioconductor package reviewer, please see Reviewers Expectations.

PeteHaitch commented 2 years ago

Hi @nilshoffmann,

Congratulations 🎉 I'm marking the package as accepted. Thank you for your contribution to Bioconductor.

I would still recommend running styler::style_pkg(filetype = "Rmd") on your package to make some cosmetic changes to the code in the vignette because it seems like these weren't incorporated when you manually applied the patch.

Cheers, Pete

lshep commented 2 years ago

The master branch of your GitHub repository has been added to Bioconductor's git repository.

To use the git.bioconductor.org repository, we need an 'ssh' key to associate with your github user name. If your GitHub account already has ssh public keys (https://github.com/nilshoffmann.keys is not empty), then no further steps are required. Otherwise, do the following:

  1. Add an SSH key to your github account
  2. Submit your SSH key to Bioconductor

See further instructions at

https://bioconductor.org/developers/how-to/git/

for working with this repository. See especially

https://bioconductor.org/developers/how-to/git/new-package-workflow/ https://bioconductor.org/developers/how-to/git/sync-existing-repositories/

to keep your GitHub and Bioconductor repositories in sync.

Your package will be included in the next nigthly 'devel' build (check-out from git at about 6 pm Eastern; build completion around 2pm Eastern the next day) at

https://bioconductor.org/checkResults/

(Builds sometimes fail, so ensure that the date stamps on the main landing page are consistent with the addition of your package). Once the package builds successfully, you package will be available for download in the 'Devel' version of Bioconductor using BiocManager::install("rgoslin"). The package 'landing page' will be created at

https://bioconductor.org/packages/rgoslin

If you have any questions, please contact the bioc-devel mailing list (https://stat.ethz.ch/mailman/listinfo/bioc-devel); this issue will not be monitored further.