Bioconductor / Contributions

Contribute Packages to Bioconductor
134 stars 33 forks source link

plyinteractions #3194

Closed js2264 closed 11 months ago

js2264 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 @js2264

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: plyinteractions
Title: Extending tidy verbs to genomic interactions
Description: Operate on `GInteractions` objects as tabular data using 
    `dplyr`-like verbs. The functions and methods in `plyinteractions`
    provide a grammatical approach to manipulate `GInteractions`, to 
    facilitate their integration in genomic analysis workflows. 
Version: 0.99.0
Date: 2023-08-21
Authors@R: 
    c(person("Jacques", "Serizay", , "jacquesserizay@gmail.com",
        role = c("aut", "cre")
    ))
License: Artistic-2.0
URL: https://github.com/js2264/plyinteractions
BugReports: https://github.com/js2264/plyinteractions/issues
biocViews: Software,
    Infrastructure
Encoding: UTF-8
Roxygen: list(markdown = TRUE)
Imports:
    InteractionSet,
    GenomeInfoDb, 
    BiocGenerics, 
    GenomicRanges,
    plyranges, 
    IRanges,
    S4Vectors,
    rlang, 
    dplyr, 
    tibble,
    tidyselect,  
    methods, 
    utils
Suggests: 
    rtracklayer,
    BiocStyle,
    covr,
    knitr,
    rmarkdown,
    sessioninfo,
    testthat (>= 3.0.0),
    RefManageR
Config/testthat/edition: 3
VignetteBuilder: knitr
RoxygenNote: 7.2.3
Collate: 
    'ginteractions-getters.R'
    'DelegatingGInteractions-class.R'
    'PinnedGInteractions-class.R'
    'AnchoredPinnedGInteractions-class.R'
    'GroupedGInteractions-class.R'
    'anchor.R'
    'annotate.R'
    'arrange.R'
    'count-overlaps.R'
    'count.R'
    'data.R'
    'filter-overlaps.R'
    'filter.R'
    'find-overlaps.R'
    'flank.R'
    'ginteractions-construct.R'
    'ginteractions-env.R'
    'ginteractions-scoping.R'
    'ginteractions-setters.R'
    'tbl_vars.R'
    'group_data.R'
    'group_by.R'
    'internals.R'
    'join-overlap-left.R'
    'methods-show.R'
    'mutate.R'
    'pin.R'
    'reexports-dplyr.R'
    'reexports-plyranges.R'
    'reexports.R'
    'rename.R'
    'replace-anchors.R'
    'select.R'
    'shift.R'
    'slice.R'
    'stretch.R'
    'summarize.R'
LazyData: true
Depends: 
    R (>= 2.10)
bioc-issue-bot commented 1 year ago

Your package has been added to git.bioconductor.org to continue the pre-review process. A build report will be posted shortly. Please fix any ERROR and WARNING in the build report before a reviewer is assigned or provide a justification on why you feel the ERROR or WARNING should be granted an exception.

IMPORTANT: Please read this documentation for setting up remotes to push to git.bioconductor.org. All changes should be pushed to git.bioconductor.org moving forward. It is required to push a version bump to git.bioconductor.org to trigger a new build report.

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 the Bioconductor Build System.

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.

The following are build products from R CMD build on the Bioconductor Build System: Linux (Ubuntu 22.04.2 LTS): plyinteractions_0.99.0.tar.gz macOS 12.6.5 Monterey: plyinteractions_0.99.0.tar.gz

Links above 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/plyinteractions 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: 56d354b6315c4116a8ac0ef283ef78aec4a01838

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 the Bioconductor Build System.

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

Please see the build report for more details.

The following are build products from R CMD build on the Bioconductor Build System: Linux (Ubuntu 22.04.2 LTS): plyinteractions_0.99.1.tar.gz macOS 12.6.5 Monterey: plyinteractions_0.99.1.tar.gz

Links above 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/plyinteractions 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: 0b6c271d5732ed02f3da8202413fbc48d249db74

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 the Bioconductor Build System.

On one or more platforms, the build results were: "ERROR, skipped". 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.

The following are build products from R CMD build on the Bioconductor Build System: Linux (Ubuntu 22.04.2 LTS): plyinteractions_0.99.2.tar.gz

Links above 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/plyinteractions to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 12 months ago

A reviewer has been assigned to your package for an indepth review. Please respond accordingly to any further comments from the reviewer.

js2264 commented 12 months ago

Hi @HelenaLC thanks for reviewing this package.

Build on linux is all green, but it errors on MacOS when building the vignette, specifically the process_pairs.Rmd vignette, at the step where a pairs file is fetched from ExperimentHub (id EH7707) using the HiContactsData package. I suspect that there might be an issue with this recently updated file, but I cannot figure out what fails. When I try running bioconductor devel docker in the cloned repo on a local Mac, things work:

BiocManager::install("tidyomics/plyinteractions", force = TRUE)
library(plyinteractions)
devtools::build_vignettes()
## This finishes ok

Any idea why it the vignette build would fail specifically on Mac, when trying to fetch a resources from ExperimentHub? Thanks, J

HelenaLC commented 12 months ago

Trying to build the package on my personal laptop ('23 arm64 MacBook) I am getting this...

processing file: process_pairs.Rmd                                                                                                         
Quitting from lines 47-58 [importPairs] (process_pairs.Rmd)
Error in `vroom_()`:
! bad value
Backtrace:
 1. rlang::set_names(...)
 2. vroom::vroom(pairs_file, delim = "\t", col_names = FALSE, comment = "#")
 3. vroom:::vroom_(...)
Execution halted

...google also found this saying "bad value", so (I think) it might be something vroom-related, and hopefully it will be fixed by 'them' eventually.

bioc-issue-bot commented 12 months ago

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

bioc-issue-bot commented 12 months ago

Dear Package contributor,

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

Your package has been built on the Bioconductor Build System.

On one or more platforms, the build results were: "ERROR, skipped". 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.

The following are build products from R CMD build on the Bioconductor Build System: ERROR before build products produced.

Links above 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/plyinteractions to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 12 months ago

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

bioc-issue-bot commented 12 months ago

Dear Package contributor,

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

Your package has been built on the Bioconductor Build System.

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

Please see the build report for more details.

The following are build products from R CMD build on the Bioconductor Build System: Linux (Ubuntu 22.04.2 LTS): plyinteractions_0.99.4.tar.gz macOS 12.6.5 Monterey: plyinteractions_0.99.4.tar.gz

Links above 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/plyinteractions to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

js2264 commented 12 months ago

Trying to build the package on my personal laptop ('23 arm64 MacBook) I am getting this...

processing file: process_pairs.Rmd                                                                                                         
Quitting from lines 47-58 [importPairs] (process_pairs.Rmd)
Error in `vroom_()`:
! bad value
Backtrace:
 1. rlang::set_names(...)
 2. vroom::vroom(pairs_file, delim = "\t", col_names = FALSE, comment = "#")
 3. vroom:::vroom_(...)
Execution halted

...google also found this saying "bad value", so (I think) it might be something vroom-related, and hopefully it will be fixed by 'them' eventually.

Thanks @HelenaLC for your input, I've dropped vroom in favor of read.delim, takes much longer but it works cross-platform. Let me know if you need anything else while reviewing the code!

HelenaLC commented 12 months ago

code

documentation

vignette

other

js2264 commented 12 months ago

Thanks @HelenaLC for the review! I'm now working on it. I'll ping you when I have answered all your comments.

code

  • [x] avoid the use of paste() in condition signals (internals.R line 5); it's not necessary (e.g., stop("hello", "world") works)
  • [x] for clarity, we recommend S4 class and generic definitions to be placed in a single file: AllClasses.R and AllGenerics.R, respectively (see here)
  • [x] validity checks on the S4 classes defined in the package are currently missing; e.g., running (ggi=GroupedGInteractions) ggi@group_keys <- DataFrame() and break anything afterwards - not that anyone would ever do this, but S4 class definitions should generally come with a sensible validity method for long-term development/maintenance purposes and against undesirable user-behavior (see 1.3 here for a minimal example) - e.g., here, n_groups() should be length 1, names(group_keys()) should occur in the data, nrow(group_keys()) should match n_groups(), etc.

documentation

  • [x] the package currently Depends: R (>= 2.10), however, it will not be installable under anything less than R v4.3 (Bioc v3.18); I'd suggest bumping R version requirements accordingly (note also that R's native pipe operator |> wasn't available until recently, so this won't work with R v2.1 in any case!)
  • [x] LazyData: should be set to false or removed (see here for details)
  • [x] consider adding a package .man page (i.e., ?plyinteractions), e.g., describing the package's generally functionality (see other packages for examples)
  • [x] in the BiocCheck() report, there are NOTEs related to the number of characters in examples (> 100 characters make things hard to read when viewing .man. pages via ?...); might be worth considering 1. inserting a few additional line breaks, and 2. not using spaces around =s to keep things more compact (this is also the recommended code style in general)

vignette

  • [x] type line 184: dependant > dependent
  • [x] please add a brief # Introduction section (see here for details)
  • [x] no 'issue' per se, but I find the plyinteractions.Rmd vignette very hard to digest, primarily because there is a lot of lengthy code output and comparability little text; while thorough documentation is highly appreciated, perhaps consider compacting this to something more readable (to give one example, many tidy verbs form groups with similar behavior, and it might be sufficient to list those and only demo/evaluate an example; e.g., the code chunk at line 444 executes 14 things - without any comment on what's happening in each - is this really necessary? wouldn't it be more informative to name (not execute) relevant functions and highlight selected examples instead?

other

  • [x] I strongly urge you to consider implementing unit tests; these are indispensable for both package development and maintenance (testthat is currently listed in the DESCRIPTION, but I don't see any tests)
  • [x] consider adding a NEWS file to keep track of changes to the code from one version to the next
  • [x] the scripts at data-raw/ should be placed under inst/scripts/ (see here for details)
bioc-issue-bot commented 12 months ago

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

bioc-issue-bot commented 12 months ago

Dear Package contributor,

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

Your package has been built on the Bioconductor Build System.

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

Please see the build report for more details.

The following are build products from R CMD build on the Bioconductor Build System: macOS 12.6.5 Monterey: plyinteractions_0.99.5.tar.gz Linux (Ubuntu 22.04.2 LTS): plyinteractions_0.99.5.tar.gz

Links above 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/plyinteractions to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 12 months ago

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

js2264 commented 12 months ago

Hi @HelenaLC, I've taken all your comments into account every time it was possible. Please let me know if there is anything I can change to improve the package, thanks!

bioc-issue-bot commented 12 months ago

Dear Package contributor,

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

Your package has been built on the Bioconductor Build System.

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

Please see the build report for more details.

The following are build products from R CMD build on the Bioconductor Build System: Linux (Ubuntu 22.04.2 LTS): plyinteractions_0.99.6.tar.gz macOS 12.6.5 Monterey: plyinteractions_0.99.6.tar.gz

Links above 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/plyinteractions to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

HelenaLC commented 11 months ago

Thanks for the prompt response & action - it's come along nicely! While none of these are a necessity for acceptance, I'd like to leave you with a couple more (minor) comments that might be worth addressing prior to release, or a bit of a learning opportunity perhaps. As indicated, happy to accept in any case (!), just that fixes won't immediately come through once the package enters the Bioc build system. There's still more than a week of time to realize (some of) these, so, it's you're call - just let me know!

bioc-issue-bot commented 11 months ago

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

bioc-issue-bot commented 11 months ago

Dear Package contributor,

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

Your package has been built on the Bioconductor Build System.

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

Please see the build report for more details.

The following are build products from R CMD build on the Bioconductor Build System: Linux (Ubuntu 22.04.2 LTS): plyinteractions_0.99.7.tar.gz macOS 12.6.5 Monterey: plyinteractions_0.99.7.tar.gz

Links above 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/plyinteractions to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

js2264 commented 11 months ago

Hi @HelenaLC, thanks for your fast reivew and great suggestions. I did learn a bit while trying to improve the package following your recommendations.

Regarding validity checks, I have tried to make them robust enough. I have slightly improved and fixed some bugs in the validity checks, which limits misuse form the end user. Note that I specifically did not create dedicated constructors for the new classes (AnchoredPinnedGInteractions and GroupedGInteractions); instead, these classes are created when using group_by(), pin_by() and anchor_*(), and these methods should take care of use and edge cases (as much as I could think). Also, my understanding is that once a S4 object is created, changing it directly with @<- does not trigger the validity check, is this incorrect?

Regarding unit tests, I am currently improving them to control for expected output value rather than class. I also included some tests reflecting "real" tidy workflows. I still need to go over the dplyr-related and setters tests, this should be over by tomorrow.

[x] in the package help (plyinteractions.man) page, it would be nice to
hyperlink the functions you mention, i.e., using \code{\link{}} [x] in the vignettes, I'd suggest getting rid of the package pre-loading (lines 28-31)
because this code chunk is not being echoed, as to avoid (accidental) confusion
should a library be added/removed at any point in the future [x] in the vignettes, for clarity, I'd suggest using xxx() as opposed to
xxx (i.e., adding empty parentheses) when referring to functions rather than,
say, variables, arguments, classes etc. (of course, just my opinion, you're call!) [x] throughout the vignettes, consider using r BiocStyle::CRANpkg("dplyr") when
mentioning dplyr package (and other CRAN packages), or r BiocStyle::Githubpkg()
(should there be any mention of a GH package that I missed); this is currently under
Acknowledgments at the very end, but easy to miss/seldomly reached in my opinion... [x] appreciate some class validity checks, though these could be a little more
extensive/fail-save, ideally, really checking everything is exactly as it should...
(just a random example:) @pin <- as.integer(c(1, 2)) on a PinnedGInteractions
breaks the if-statement in AllClasses.R line 51 since @pin should be scalar
(dumb example, but S4 class definitions should ideally be very, very robust...) [x] while unit test coverage is not at 87%+ with a few hits per line, it's very minimalist -
e.g., many current tests check that a function basically works (e.g., an S4 object is returned) -
ideally, tests should cover relevant use- and edge-cases such as invalid input arguments,
more complex computations, the actual data in the output (not just its class) etc.
Testing that a function just works is no guarantee for correctness of outputs

HelenaLC commented 11 months ago

Great efforts, thanks! Re tests, yeah, would be good to cover the basics here. I recall that for tidySingleCellExperiment I basically went to dplyr/tidyr and copied & modified some of their tests. But tests are definitely something that can and should grow over time, as issues arise etc.

You're correct in that @<- won't trigger validity checks, and I don't have a strong argument here other than: it's a principle of the matter to assure classes we define remain valid, also from a developer perspective (i.e., a good validity check should prevent any functions downstream from (by accident) creating something invalid).

Not familiar with all the details, but if the 'new' classes have a dplyr class underlying them, perhaps calling their validity checks in a some way might be the easier and most comprehensive solution? Else, I think it's pretty straightforward to implement yourself.

I'll hold out until the next update/for you're 'go' to accept!

bioc-issue-bot commented 11 months ago

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

bioc-issue-bot commented 11 months ago

Dear Package contributor,

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

Your package has been built on the Bioconductor Build System.

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

Please see the build report for more details.

The following are build products from R CMD build on the Bioconductor Build System: Linux (Ubuntu 22.04.2 LTS): plyinteractions_0.99.8.tar.gz macOS 12.6.5 Monterey: plyinteractions_0.99.8.tar.gz

Links above 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/plyinteractions to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

js2264 commented 11 months ago

Thanks @HelenaLC for all your inputs and comments. I've worked a bit further on the tests. I will also continue working on improving validity tests in the coming weeks, but I won't have time to do that in the coming days, so I think it's ok to give it a go and send it to devel as it is now, if this is ok for you. Thanks again for your review!

bioc-issue-bot commented 11 months 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 11 months ago

The default branch of your GitHub repository has been added to Bioconductor's git repository as branch devel.

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/js2264.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("plyinteractions"). The package 'landing page' will be created at

https://bioconductor.org/packages/plyinteractions

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.