Bioconductor / Contributions

Contribute Packages to Bioconductor
135 stars 33 forks source link

tripr #2267

Closed iofeidis closed 3 years ago

iofeidis commented 3 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 3 years ago

Hi @iofeidis

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: tripr
Type: Package
Title: T-cell Receptor/Immunoglobulin Profiler (TRIP)  
Version: 0.99.0
Authors@R: c(
        person("Maria Th.", "Kotouza", email = "-", role = "aut"),
        person("Katerina", "Gemenetzi", email = "-", role = "aut"),
        person("Chrysi", "Galigalidou", email = "-", role = "aut"),
        person("Elisavet", "Vlachonikola", email = "-", role = "aut"),
        person("Nikolaos", "Pechlivanis", email = "-", role = "aut"),
        person("Andreas", "Agathangelidis", email = "-", role = "aut"),
        person("Raphael", "Sandaltzopoulos", email = "-", role = "aut"),
        person("Pericles A.", "Mitkas", email = "-", role = "aut"),
        person("Kostas", "Stamatopoulos", email = "-", role = "aut"),
        person("Anastasia", "Chatzidimitriou", email = "-", role = "aut"),
        person("Fotis E.", "Psomopoulos", email = "-", role = "aut"),
        person("Iason", "Ofeidis", email = "jasonof@certh.gr", role = "cre")
        )
Description: TRIP is a software framework that provides analytics services on 
    antigen receptor (B cell receptor immunoglobulin, BcR IG | T cell receptor, 
    TR) gene sequence data. It is a web application written in R Shiny. It takes
    as input the output files of the IMGT/HighV-Quest tool. Users can select to 
    analyze the data from each of the input samples separately, or the combined 
    data files from all samples and visualize the results accordingly.
License: MIT + file LICENSE
Encoding: UTF-8
LazyData: false
biocViews: 
    BatchEffect,
    MultipleComparison,
    GeneExpression,
    ImmunoOncology,
    TargetedResequencing
Imports: 
    shinyjs,
    shinyFiles,
    plyr,
    data.table,
    DT,
    stringr,
    stringdist,
    plot3D, 
    gridExtra,  
    RColorBrewer,  
    plotly,
    dplyr,
    pryr,
    config (>= 0.3.1),
    golem (>= 0.3.1),
    methods,
    grDevices,
    graphics,
    stats,
    utils
Enhances: 
    parallel
Suggests:
    BiocGenerics,
    shinycssloaders,
    tidyverse,
    BiocManager,
    Biostrings,
    xtable,
    rlist,
    motifStack,
    knitr,
    rmarkdown,
    testthat (>= 3.0.0),
    fs,
    BiocStyle,
    RefManageR,
    biocthis
Depends:
    shiny (>= 1.6.0),
    shinyBS
Collate:
    "tripr-package.R"
    "global.R"
    "helpers.R"
    "run_TRIP_without_ui.R"
    "app_config.R"
    "app_server.R"
    "app_ui.R"
    "run_app.R"
    "zzz.R"
URL: https://github.com/BiodataAnalysisGroup/tripr
BugReports: https://github.com/BiodataAnalysisGroup/tripr/issues
BiocType: Software
RoxygenNote: 7.1.1
VignetteBuilder: knitr
Config/testthat/edition: 3
bioc-issue-bot commented 3 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 3 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/tripr to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 3 years ago

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

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

bioc-issue-bot commented 3 years ago

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

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

iofeidis commented 3 years ago

Hello @lshep,

Last build push (0.99.2) was made for eliminating the seq_len() NOTE of BiocCheck().

lshep commented 3 years ago

Thank you for your submission. Please find your initial review below:

Documentation

CITATION.cff

LICENSE/LICENSE.md

README.md/README.Rmd

vignettes

Listening on http://127.0.0.1:7110 Warning: Error in $: $ operator is invalid for atomic vectors [No stack trace available] Warning: Error in $: $ operator is invalid for atomic vectors 103:



- [ ] When running the shiny app is there a way to change the output directory?

- [ ] It would be nice if you can change the output directory, to also be able
  to change the "previous session" to allow for selection of a past section? is
  this possible or does it always do the last session only?

- [ ] When running via command line in the vignette, Please change the output
  directory to use a `tempdir()`. `output_path <- file.path(tempdir(),
  "myoutput")`

**R**

- [ ] In your `zzz.R` it seems like you make an output folder in the
  installation folder for tripr. This should be avoided and should use a
  tempdir() by default.  Not all users will have access to the installation
  folder especially if R or the packages were installed as root.

- [ ] run_TRIP seems like it could benefit from some argument checking to make
  sure the user enters valid arguments especially when the arguments have
  limited options.

- [ ] run_TRIP is also a very long function. In the future it might benefit to
  break sections into helper functions for earier reading and understanding of
  code.

- [ ] Please remove commented out code in R files. Notes and comments are
  encouraged but unused code and debugging code should be removed.

- [ ] In helpers.R please use message instead of cat so users can optionaly
  suprress.

- [ ] Large sectios of repeated code should be avoided. It seems a lot of the
  helper functions are closely relate and share large amounts of code
  (examples but should be applied throughout the code:  imgtfilterLow/imgtfilter
  and imgtcleaning/imgtcleaningLow.  These should be one function with an
  argument to control the differences between the functions. On quick glance I'm
  not finding many differences in the code base. Not only does this shorten and
  simplify the code, if a bug or correction should be made then its in one
  location instead of many.

- [ ] Similarly there are large amounts of repeated code in app_server.R and
  run_TRIP_without_ui.R. The sections of code that are repeated ideally would be
  in an internal function that both of these files call. Simplifing the code.

- [ ] In global. similarly to zzz.R, It should not be assumed the user has
  access to write to the installation directory and it is somewhat bad practice
  to write to that directory. We highly recommend using a tempdir as default and
  allow the user to change to a directory of their choice.

Please address the above issues.  When ready please bump the version to trigger a new build report and comment back here commenting on how the above have been addressed or any additional comments and requesting a re-review.  Cheers
iofeidis commented 3 years ago

Hi @lshep!

Thank you for reviewing our package and making suggestions! We will look into them and come back with the corresponding changes.

Cheers

bioc-issue-bot commented 3 years ago

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

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

iofeidis commented 3 years ago

Hi @lshep ,

See below our comments on the review:

CITATION.cff

  • [ ] Is this file necessary?

Although it's not necessary for the code, we would prefer to have this for the Github repo citation, as recommended by the best practices. If this poses a serious issue, we could move it to another branch, but it would be less than ideal.

LICENSE/LICENSE.md

  • [X] Are both of these files necessary?

Only LICENSE is needed. The other file is now removed.

README.md/README.Rmd

  • [X] Are both of these files necessary? Is there a concern for one getting out of date of the other?

The README.Rmd file was used for the initial creation of .md - it has been now removed.

vignettes

  • [ ] When I run the tripr::run_app() I get the following Warnings, is there concern?

    tripr::run_app()

    Listening on http://127.0.0.1:7110 Warning: Error in $: $ operator is invalid for atomic vectors [No stack trace available] Warning: Error in $: $ operator is invalid for atomic vectors 103:

This warning is due to the $ operator on shiny apps. It has no effect on the main functionality, so it can be safely ignored.

  • [X] When running the shiny app is there a way to change the output directory?

In shiny app, we have now made the default output directory based on tempdir(), and we expect the user to access its files through the Download button. However, if need be, we can add an explicit choice of selecting the output directory from the UI before pressing the button (although this would not be optimal). In either case, the explicit choice of output folder is already done in the run_TRIP() function.

  • [ ] It would be nice if you can change the output directory, to also be able to change the "previous session" to allow for selection of a past section? is this possible or does it always do the last session only?

So far the tool has been designed to work only with the previous session. However, it's in our future plans to enable support for all past sessions as well.

  • [X] When running via command line in the vignette, Please change the output directory to use a tempdir(). output_path <- file.path(tempdir(), "myoutput")

Done.

R

  • [X] In your zzz.R it seems like you make an output folder in the installation folder for tripr. This should be avoided and should use a tempdir() by default. Not all users will have access to the installation folder especially if R or the packages were installed as root.

Done.

  • [X] run_TRIP seems like it could benefit from some argument checking to make sure the user enters valid arguments especially when the arguments have limited options.

Done.

  • [ ] run_TRIP is also a very long function. In the future it might benefit to break sections into helper functions for earier reading and understanding of code.

We have already started a discussion about a refactoring of the codebase. However, given the extend of the code, this will require substantial time and therefore it will probably be addressed in future versions of the package.

  • [X] Please remove commented out code in R files. Notes and comments are encouraged but unused code and debugging code should be removed.

Done.

  • [X] In helpers.R please use message instead of cat so users can optionaly suppress.

cat() is used for printing into the logFile, not for messages on the console, so it needs to be kept.

  • [ ] Large sectios of repeated code should be avoided. It seems a lot of the helper functions are closely relate and share large amounts of code (examples but should be applied throughout the code: imgtfilterLow/imgtfilter and imgtcleaning/imgtcleaningLow. These should be one function with an argument to control the differences between the functions. On quick glance I'm not finding many differences in the code base. Not only does this shorten and simplify the code, if a bug or correction should be made then its in one location instead of many.

As mentioned before, we do intend to restructure our codebase for shortening and simplification, but in order to ensure that no bugs/issues slip through the process, it will be addressed in a future version of the package.

  • [ ] Similarly there are large amounts of repeated code in app_server.R and run_TRIP_without_ui.R. The sections of code that are repeated ideally would be in an internal function that both of these files call. Simplifing the code.

Same as before.

  • [X] In global. similarly to zzz.R, It should not be assumed the user has access to write to the installation directory and it is somewhat bad practice to write to that directory. We highly recommend using a tempdir as default and allow the user to change to a directory of their choice.

Done.

The latest push includes the aforementioned changes. Looking forward to your re-review.

Cheers!

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

lshep commented 3 years ago

We highly recommend the refactoring and simplifying of code; I strongly encourage to implement this as stated in a future update to the package as it will help with bugs and maintainability.

iofeidis commented 3 years ago

Thank you @lshep for reviewing our package!

We will work into refactoring the codebase as stated.

Regarding the package itself, how can a user now install tripr through BiocManager?

lshep commented 3 years ago

In a few days the package will be added to the daily manifest and build on the daily builder; once it builds on the daily builder it will be available through BiocManager

vjcitn commented 3 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/iofeidis.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("tripr"). The package 'landing page' will be created at

https://bioconductor.org/packages/tripr

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.