Bioconductor / Contributions

Contribute Packages to Bioconductor
134 stars 33 forks source link

planttfhunter #2835

Closed almeidasilvaf closed 1 year ago

almeidasilvaf commented 1 year 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 1 year ago

Hi @almeidasilvaf

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: planttfhunter
Title: Identification and classification of plant transcription factors
Version: 0.99.0
Date: 2022-03-10
Authors@R: 
    c(
    person(given = "Fabrício",
 family = "Almeida-Silva",
 role = c("aut", "cre"),
 email = "fabricio_almeidasilva@hotmail.com",
 comment = c(ORCID = "0000-0002-5314-2964")),
    person(given = "Yves",
 family = "Van de Peer",
 role = "aut",
 email = "yves.vandepeer@psb.vib-ugent.be",
 comment = c(ORCID = "0000-0003-4327-3730"))
    )
Description: planttfhunter is used to identify plant transcription factors (TFs) 
    from protein sequence data and classify them into families and subfamilies
    using the classification scheme implemented in PlantTFDB.
    TFs are identified using pre-built hidden Markov model profiles
    for DNA-binding domains. Then, auxiliary and forbidden domains are
    used with DNA-binding domains to classify TFs into families and
    subfamilies (when applicable). Currently, TFs can be classified in
    58 different TF families/subfamilies.
License: GPL-3
URL: https://github.com/almeidasilvaf/planttfhunter
BugReports: https://support.bioconductor.org/t/planttfhunter
biocViews: Software,
    Transcription,
    FunctionalPrediction,
    GenomeAnnotation,
    FunctionalGenomics,
    HiddenMarkovModel,
    Sequencing,
    Classification
Encoding: UTF-8
Roxygen: list(markdown = TRUE)
RoxygenNote: 7.2.0
SystemRequirements: HMMER <http://hmmer.org/>
Imports:
    Biostrings,
    SummarizedExperiment,
    utils,
    methods
Suggests: 
    BiocStyle,
    covr,
    sessioninfo,
    knitr,
    rmarkdown,
    testthat (>= 3.0.0)
Config/testthat/edition: 3
VignetteBuilder: knitr
Depends: 
    R (>= 4.2.0)
LazyData: false
almeidasilvaf commented 1 year ago

Package resubmission after name change, as suggested by @vjcitn in #2811.

almeidasilvaf commented 1 year ago

Hi, @vjcitn

I submitted this package 6 days ago, before the deadline for possible inclusion on Bioc 3.16, and it is still awaiting moderation. Other submissions after this one already have a reviewer assigned.

I have already changed the name, as you proposed in #2811. Is there anything else I need to do?

Best, Fabricio

bioc-issue-bot commented 1 year 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 1 year 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/planttfhunter to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

lazappi commented 1 year ago

Hi @almeidasilvaf

Thanks for submitting planttfhunter :tada:! Below is my review of your package. Please reply here if anything is unclear or needs any further explanation.

What next?

Please address the comments as best as you can. When you are ready for me to check the package again please reply to let me know with a summary of changes you have made or any other responses. You can use the "Quote reply" option in the ... menu on this comment to reply directly to my points below.

Luke

Review

Key: :rotating_light: Required :warning: Recommended :green_circle: Optional :question: Question

Documentation

Vignette

Man pages

Unit tests

Code

R

work_dir <- tempdir()
file1 <- file.path(work_fir, "file1.ext")
file2 <- file.path(work_fir, "file2.ext")

Additional files

bioc-issue-bot commented 1 year ago

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

bioc-issue-bot commented 1 year 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/planttfhunter to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 1 year ago

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

bioc-issue-bot commented 1 year 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/planttfhunter to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

almeidasilvaf commented 1 year ago

Hi, @lazappi

Thank you for your time to review this package. Here are my replies to each of your points.

Documentation

Vignette

  • [x] rotating_light Please mention these in the Introduction

    • [x] rotating_light Motivation for including your package in Bioconductor

Done. I included more details on how this package interoperates with other Bioconductor packages.

  • [x] rotating_light Existing package that have similar functionality and comparison to them (if applicable)

Not applicable.

  • [x] warning I think most of the functionality depends on having HMMER, it would be good to be clearer about which functions are HMMER wrappers and which are new functionality of your package

That was actually a mistake I made in the vignette. The only function that requires HMMER is annotate_pfam(), and this was already indicated in the vignette. There was an if statement to check if HMMER was installed for the function classify_tfs, but it should not have been there, as classify_tfs() does not depend on HMMER. I've also added a sentence explaining that the wrapper function get_tf_counts(), which does everything in a single step (from a list of AAStringSet objects to a SummarizedExperiment of TF counts), also requires HMMER, as it uses annotate_pfam under the hood.

  • [x] warning I think the solution for using example data in the package when HMMER isn't available works but it would be good to explain in the text that is the case so it's clear for users

Done. I added a note to make this clear.

Man pages

  • [x] green_circle It is recommended to add a package man page usethis::use_package_doc()

Done.

Unit tests

  • [x] warning I think its probably better to use testthat::expect_s3_class() or testthat::expect_type() rather than testthat::expect_equal(class(object), "class")

Disagree, as they are not the same. For instance, the following test would fail:

m <- matrix(c(1,2,3,4), ncol=2)
expect_type(m, "matrix")

Thus, I did not change the tests. The package has test coverage of 100%, anyway.

Code

R

  • [x] warning When creating temporary files I think it's probably better to use tempfile() directly rather than file.path(tempdir(), "file.name") or when you are creating multiple files just get the temp directory once like:
work_dir <- tempdir()
file1 <- file.path(work_fir, "file1.ext")
file2 <- file.path(work_fir, "file2.ext")

Done.

  • [X] green_circle I wonder if there is a better way to encode the different labels in the family checking functions? I'm not really familiar with these but I would be worried that I could easily get some of the conditions in the wrong order. I think a switch statement could maybe be used for some of the simpler ones but not sure about the more complex cases.

I wondered the same thing during the development of this package. switch statements would be helpful in a tiny fraction of them, because classification schemes are complex (they vary based on number of domains, and on the presence/absence of auxiliary domains). One could get them in wrong order, but that's what unit tests are for, right?

  • [x] green_circle Maybe it would be clearer to rename is_valid() to something like is_valid_cmd() so the name tells you want kind of validity it is checking?

Done.

Additional files

  • [x] warning There is non-standard dev/ directory in the repository. It seems to have been created by {biocthis} though and is included in .Rbuildignore so it could be ok. Maybe @Bioconductor/core can clarify?

This is an old discussion (see here, for instance: https://github.com/lcolladotor/biocthis/issues/21). There is no problem with /dev directories, and they are present in several Bioc packages. Besides, as you rightly pointed out, they are listed in .Rbuildignore, so neither R CMD check nor BiocCheck() flags them as a problem.

Best, Fabricio

lazappi commented 1 year ago

This looks good from my point to view now. The only reason I haven't marked this as accepted is because of the dev/ directory. I know this has been discussed before (and personally I think it's fine) but I think the official position is still that these shouldn't be included so I would like to hear from @Bioconductor/core on whether this is ok or not.

lshep commented 1 year ago

Yes it is okay. If you are comfortable please accept.

bioc-issue-bot commented 1 year 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.

lshep commented 1 year 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/almeidasilvaf.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("planttfhunter"). The package 'landing page' will be created at

https://bioconductor.org/packages/planttfhunter

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.

lshep commented 1 year 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/almeidasilvaf.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("planttfhunter"). The package 'landing page' will be created at

https://bioconductor.org/packages/planttfhunter

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.