Bioconductor / Contributions

Contribute Packages to Bioconductor
134 stars 33 forks source link

katdetectr #2676

Closed daanhazelaar closed 2 years ago

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

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: katdetectr
Title: Detection, Characterization and Visualization of Kataegis in Sequencing Data
Version: 0.99.0
Authors@R: c(
    person("Daan", "Hazelaar", , "daanhazelaar@gmail.com", role = c("aut", "cre"),
 comment = c(ORCID = "0000-0002-7513-6813")),
    person("Job", "van Riet", , "j.vanriet@erasmusmc.nl", role = c("aut"),
 comment = c(ORCID = "0000-0001-7767-7923")),
    person("Harmen", "van de Werken", , "h.vandewerken@erasmusmc.nl", role = c("ths"),
 comment = c(ORCID = "0000-0002-9794-1477"))
  )
Description: Kataegis refers to the occurrence of regional hypermutation and is
    a phenomenon observed in a wide range of malignancies. Using changepoint detection
    katdetectr aims to identify putative kataegis foci from common data-formats housing genomic variants.
    Katdetectr has shown to be a robust package for the detection, characterization and visualisation of kataegis.
License: GPL-3 + file LICENSE
URL: https://github.com/ErasmusMC-CCBC/katdetectr
BugReports: https://github.com/ErasmusMC-CCBC/katdetectr
Depends:
    R (>= 4.1.0)
Imports: 
    base (>= 4.1.3),
    BiocParallel (>= 1.26.2),
    BiocStyle (>= 2.20.0),
    changepoint (>= 2.2.3),
    changepoint.np (>= 1.0.3),
    checkmate (>= 2.0.0),
    dplyr (>= 1.0.8),
    GenomicRanges (>= 1.44.0),
    GenomeInfoDb (>= 1.28.4),
    IRanges (>= 2.26.0),
    maftools (>= 2.10.5),
    methods (>= 4.1.3),
    rlang (>= 1.0.2),
    S4Vectors (>= 0.30.2),
    tibble (>= 3.1.6),
    VariantAnnotation (>= 1.38.0),
    Biobase (>= 2.54.0),
    Rdpack (>= 2.3),
    ggplot2 (>= 3.3.5),
    tidyr (>= 1.2.0),
    BSgenome (>= 1.62.0)
Suggests: 
    BSgenome.Hsapiens.UCSC.hg38 (>= 1.4.4),
    ggtext (>= 0.1.1),
    scales (>= 1.2.0),
    knitr (>= 1.37),
    rmarkdown (>= 2.13),
    testthat (>= 3.0.0),
    BSgenome.Hsapiens.UCSC.hg19 (>= 1.4.3)
RdMacros: Rdpack
VignetteBuilder: 
    knitr
biocViews: WholeGenome, Software, SNP, Sequencing, Classification, VariantAnnotation
Encoding: UTF-8
Roxygen: list(markdown = TRUE)
RoxygenNote: 7.1.2
Config/testthat/edition: 3
vjcitn commented 2 years ago

maftools produces a rainfall plot https://www.bioconductor.org/packages/devel/bioc/vignettes/maftools/inst/doc/maftools.html

any chance you can reuse that or other related functions already in bioconductor?

daanhazelaar commented 2 years ago

Dear vjcitn,

The rainfall plot function of katdetectr is build around the ggplot2 package. We prefer using the tidy format for organising our data and subsequent visualisation. Maftools rainfall plot is generated using R graphics package which does not fit well into the workflow of katdetectr. We have not found a related function on bioconductor that generates a rainfall plot (or something similar) and fits well into a tidy format workflow. Therefore, we prefer using katdetectrs custom rainfall plot function.

Kind regards, Daan Hazelaar

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

daanhazelaar commented 2 years ago

Dear reviewer,

I see the WARNINGS originate from undocumented code objects. However, all mentioned functions and classes are documented using roxygen2. I also do not get these WARNINGS when running CMD check my self (both on linux en macOS). Can you perhaps give a suggestion on how to fix these WARNINGS?

kind regards, Daan Hazelaar

lazappi commented 2 years ago

Hmmm...that's a bit weird. Can you check that the documentation has been generated by {roxygen2} (probably with devtools::document()) and that you have committed the files to your git repository (and pushed them to the Bioconductor remote)? I think the most likely explanation is that you have them locally but for some reason they haven't made it to the build system. You can also try cloning from the Bioconductor git address above to see exactly what is there.

daanhazelaar commented 2 years ago

Dear lazappi,

Aha for some reason the man folder generated by roxygen2 was not included in the git repository. I think that was the problem. I have included it now but I can't push to the bioconductor remote. I followed all steps of the manual and all credentials seem correct to me. Can you perhaps tell me why I'm denied writing acces to git@git.bioconductor.org:packages/katdetectr.git?

Screenshot 2022-05-31 at 10 32 24

lazappi commented 2 years ago

You need to push to upstream master, the Bioconductor git server still uses master as the main branch rather than main.

daanhazelaar commented 2 years ago

Now I get the following. I have not been able to solve it. Do you have any advice?

Screenshot 2022-05-31 at 13 05 30

lshep commented 2 years ago

Please see #5 of new package workflow .

bioc-issue-bot commented 2 years ago

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

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

nturaga commented 2 years ago

You seemed to be able to push now! Great news!

daanhazelaar commented 2 years ago

Dear nturaga,

Yes definitely great! 😅

Kind regards, Daan Hazelaar

lazappi commented 2 years ago

Hi @daanhazelaar

Thanks for submitting katdetectr :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.

Luke

Review

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

General package development

DESCRIPTION file

NAMESPACE file

Package data

Documentation

Vignette

Man pages

Code

R

daanhazelaar commented 2 years ago

Dear Luke,

Apologies for my late reply, I missed your previous comment. Thanks for your review and feedback! I'm working on implementing it.

However, I have encountered a strange problem. When I run R CMD check on the current version of katdetectr (as it is available on the bioconductor repo now) which has been build on linux, Windows and Mac without errors and warnings. I get the following warning:

Screenshot 2022-06-20 at 19 48 23

It seems to be related to the .Rd files in the man folder. When I delete the man folder (and do not run devoots::document()) then I don't get this error. When I call devools::document() (which generates the man directory) en run R CMD check I again get the same error.

Do you know where this comes from? Some advice would be great!

Kind regards, Daan Hazelaar

lazappi commented 2 years ago

Hi @daanhazelaar

I noticed that as well when I was reviewing your package. I think it is something that has changed in {rtracklayer}. I can see the same warning just from loading {rtracklayer} (using the Bioconductor development versions):

#> library(rtracklayer)
# Warning message:
# replacing previous import ‘utils::download.file’ by ‘restfulr::download.file’ when loading ‘rtracklayer’  

I think for now don't worry too much about this and try to address the other comments I made, once those have been addressed we can see if you still need to do something about this.

daanhazelaar commented 2 years ago

Hi Luke,

Thanks for your reply. Updating to R 4.2 made this warning disappear. I'll continue with implementing your other remarks.

Kind regards, Daan Hazelaar

bioc-issue-bot commented 2 years ago

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

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: "WARNINGS". 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/katdetectr 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: 06b1334b6b04cfcf5f6f31785d737cd442e95c24

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

daanhazelaar commented 2 years ago
daanhazelaar commented 2 years ago

Dear @lazappi ,

I have addressed your comments and summed up my reply or solution in the previous comment. I'm curious towards your feedback.

Kind regards, Daan Hazelaar

lazappi commented 2 years ago
  • Please address as many of the notes from BiocCheck::BiocCheck() as possible o NOTE: Cannot determine whether maintainer is subscribed to the Bioc-Devel mailing list (requires admin credentials).  I’m subscribed for the Bioc-Devel mailing list

If this is happening locally it's ok, if it's happening on the build system it will need to be looked into

o Usually, the BugReports URL points directly to the GitHub issues page  Done

I think here I meant https://github.com/ErasmusMC-CCBC/katdetectr/**issues** but it's not a big deal

  • Package data o Could you access the APL_primary.maf file directly from {maftools} rather than copying it?  I also use this file for many of the unit tests. If the maintainer of maftools decides to make an adjustment to this file than my test will likely fail. Therefor I prefer keeping it as it is now.

Hmmm...ok. I still think it's better not to duplicate things and the file is probably unlikely to change but I can see this could be an issue.

  • Documentation
  • Vignette: o Please add a table of contents  Done

I can't see this. You need to set toc: TRUE in the YAML frontmatter.

o Please add an Introduction section. This should include: Motivation for inclusion in Bioconductor. Comparison to similar packages (if applicable)  Done, I included this in the introduction. However, we are working on publishing an application note for katdetectr which includes extensive performance evaluation of katdetectr and other kataegis detection packages (maftools, seqkat, ClusteredMutations, Kataegis and SigProfilerClustered). When this paper is published, I will of course link to this paper in the vignette. I personally think it’s not necessary to include performance evaluation / comparison to other packages in our vignette.

No, you definitely don't need to include a full performance comparison here. We just want maintainers to acknowledge existing similar packages and mention any important differences between them and the new package.

o Please add an Installation section. This should use Bioconductor installation instructions and NOT be evaluated.  Done

I think suggesting people install development versions from GitHub is a bit of a grey area. Once the package is accepted both the development and release versions will be available from Bioconductor. (sidenote: you only need the {remotes} package to install from GitHub rather than the (much bigger) {devtools} package).

o Accessor function should be used rather than @ or slot() (except when the object comes from another package which does not provide them)  Done, except for cptChromosome@cpts (file: function_performChangepointDetection line 67) as Changepoint::cpts() does not give the same output as object@cpts so I still use object@cpts in this case. Also maf@data (file function_importGenomicVariants line 73) as maftools does not provide a getter method for this slot. I also kept object@slot in the tests (is this ok?) I prefer this for readability.

There are some potential problems with using @ in tests which is that if something changes in the getter function that won't be transferred to your tests and you could get results that don't match what would happen for a user.

o You should allow users to provide a BiocParallel rather than just a number of workers, this allows the user to control how the parallelisation is done  Done

Generally it's best to use BiocParallel::SerialParam() as the default because it should be fine on any system and doesn't accidentally use multiple cores.

  • Question: o browseVignettes() does not work when the vignettes are placed in the vignettes directory. How does a user easily access the vignette of katdetectr?

That should work but you probably have to fully install the package. Personally I usually find it easier to look at the rendered version on the Bioconductor website but this should also work.

I think you are pretty close, just a few minor things. Just so you know I will be going on leave next week so likely won't be able to make any further comments until mid-July (possibly later).

daanhazelaar commented 2 years ago

Dear @lazappi

Thanks for your reply. I'll implement your comments. I hope you have a great time on your leave.

Do you think you'll be able to review katdetectr if I implant your comments today? I'll be giving a presentation regarding katdetectr and its performance comparison next week Wednesday. It would be great if I can add a slide with: accepted on Bioconductor! 🎉

Kind regards Daan Hazelaar

lazappi commented 2 years ago

If you can make changes today I will try to look at it one more time but no guarantees it will be approved by then.

bioc-issue-bot commented 2 years ago

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

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

daanhazelaar commented 2 years ago

Dear @lazappi

It would be great if you can find the time for a review before your leave. Regardless, I hope you have a nice time on your leave!

Hereby my reply to your comments:

• Regarding my subscription to the Bioc-Devel mailing list. The build system does recognise me as a subscriber.

• I added a link to the issues page of katdetect. I thought I did that already but now it’s there for sure.

• Regarding the MAF data set from maftools. I hope it’s ok to keep it as is.

• I added toc:TRUE to the YAML frontmatter.

• In the vignette I have acknowledged all other kataegis detection packages I know off. Which are also included in our performance evaluation.

• I removed the github install suggestion from the vignette and the README.md

• I replaced all @ symbols with its corresponding getter function in the tests

• I replaced BiocParallel::MulticoreParam(workers = 1) with BiocParallel::SerialParam()

• I removed the browseVignettes() suggestion from the README.

Kind regards, Daan Hazelaar

lazappi commented 2 years ago

• I added toc:TRUE to the YAML frontmatter.

The toc: TRUE needs to go under the output type, so something like:

output:
  BiocStyle::html_document:
    toc: true

You should be able to see the contents in the rendered vignette if it has worked correctly.

• I replaced BiocParallel::MulticoreParam(workers = 1) with BiocParallel::SerialParam()

I think it's probably best to do that in the tests as well.

lazappi commented 2 years ago

Hi @daanhazelaar

I am going to approve this now and trust you to make these final minor changes. Congratulations on getting {katdetectr} into Bioconductor 🎉 ! It can take a couple of days to be picked up by the build system but then should be available in Bioconductor devel.

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.

daanhazelaar commented 2 years ago

Hi @lazappi!

Great to hear! I'll implement these minor changes immediately. Thank for your time and effort.

Enjoy your leave!

Kind regards, Daan Hazelaar

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

https://bioconductor.org/packages/katdetectr

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.