oncoscanR - Githubissues

yannchristinat commented 2 years ago

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

Repository: https://github.com/yannchristinat/oncoscanR

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

[x] I understand that by submitting my package to Bioconductor, the package source and all review commentary are visible to the general public.
[x] I have read the Bioconductor Package Submission instructions. My package is consistent with the Bioconductor Package Guidelines.
[x] I understand Bioconductor Package Naming Policy and acknowledge Bioconductor may retain use of package name.
[x] I understand that a minimum requirement for package acceptance is to pass R CMD check and R CMD BiocCheck with no ERROR or WARNINGS. Passing these checks does not result in automatic acceptance. The package will then undergo a formal review and recommendations for acceptance regarding other Bioconductor standards will be addressed.
[x] My package addresses statistical or bioinformatic issues related to the analysis and comprehension of high throughput genomic data.
[x] I am committed to the long-term maintenance of my package. This includes monitoring the support site for issues that users may have, subscribing to the bioc-devel mailing list to stay aware of developments in the Bioconductor community, responding promptly to requests for updates from the Core team in response to changes in R or underlying software.
[x] I am familiar with the Bioconductor code of conduct and agree to abide by it.

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

[x] The 'devel' branch for new packages and features.
[x] The stable 'release' branch, made available every six months, for bug fixes.
[x] Bioconductor version control using Git (optionally via GitHub).

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

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: oncoscanR
Type: Package
Title: Meta-analyses of CNV segments
Version: 0.99.0
Authors@R: person("Yann", "Christinat", email = "yann.christinat@hcuge.ch", 
        role = c("aut", "cre"))
Description: The software uses the copy number segments from a text file and 
    identifies all chromosome arms that are globally altered and computes various 
    genome-wide scores. The following HRD scores (characteristic of BRCA-mutated cancers) 
    are included: LST, HR-LOH, nLST and gLOH. the package is made for the 
    ThermoFisher Oncoscan assay analyzed with their Chromosome Alteration Suite 
    (ChAS) but can be adapted to any input.
License: MIT + file LICENSE
Date: 2020-06-24
Encoding: UTF-8
LazyData: true
VignetteBuilder: knitr
biocViews:
    CopyNumberVariation,
    Microarray,
    Software
Imports:
    readr,
    S4Vectors,
    magrittr
RoxygenNote: 7.1.2
Depends:
    R (>= 3.6),
    IRanges (>= 2.6.1),
    GenomicRanges (>= 1.24.3)
Suggests: 
    testthat (>= 2.1.0),
    jsonlite,
    knitr,
    rmarkdown
URL: https://github.com/yannchristinat/oncoscanR
BugReports:
    https://github.com/yannchristinat/oncoscanR/issues

vjcitn commented 2 years ago

Thanks for your submission. The vignette needs work. You should have show methods for objects, not have

workflow_oncoscan.run(segs.filename, "M")
#> $armlevel
#> $armlevel$AMP
#> character(0)
#> 
#> $armlevel$LOSS
#> character(0)
#> 
#> $armlevel$LOH
#> character(0)
#> 
#> $armlevel$GAIN
#> character(0)
#> 
#> 
#> $scores
#> $scores$HRD
#> [1] "Negative, nLST=1"
#> 
#> $scores$TDplus
#> [1] 0
#> 
#> $scores$avgCN
#> [1] "2.10"
#> 
#> 
#> $gender
#> [1] "M"
#> 
#> $file
#> [1] "chas_example.txt"

in the vignette. Dumping JSON is also not desirable.

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

yannchristinat commented 2 years ago

@vjcitn Vignette has been re-worked and developped (commit pushed to git.bioconductor.org:packages/oncoscanR). But not sure what was the JSON issue as there is no JSON dump in the vignette...

vjcitn commented 2 years ago

Early on, in the previous version, we had

The script will output a JSON string into the terminal with all the computed information. :

{   "armlevel": {     "AMP": [],     "LOSS": ["17p", "2q", "4p"],     "LOH": ["14q", "5q", "8p", "8q"],     "GAIN": [19p", "19q", "1q", "20p", "20q", "3q", "5p", "6p", "9p", "9q", "Xp", "Xq"]   },   "scores": {     "HRD": "Negative, nLST=12",     "TDplus": 22,     "avgCN": "2.43"   },   "gender": "F",   "file": "H19001012_gene_list_full_location.txt" }

I will check the new one.

yannchristinat commented 2 years ago

Ok. The issue is the script not the vignette. I can remove the script from the package (it was a wrapper for a function that I found convenient but the package is perfectly functional without it)

vjcitn commented 2 years ago

For some reason it comes out better in this rendering

{
  "armlevel": {
    "AMP": [],
    "LOSS": ["17p", "2q", "4p"],
    "LOH": ["14q", "5q", "8p", "8q"],
    "GAIN": [19p", "19q", "1q", "20p", "20q", "3q", "5p", "6p", "9p", "9q", "Xp", "Xq"]
  },
  "scores": {
    "HRD": "Negative, nLST=12",
    "TDplus": 22,
    "avgCN": "2.43"
  },
  "gender": "F",
  "file": "H19001012_gene_list_full_location.txt"
}

and you can leave that if you want.

yannchristinat commented 2 years ago

triple quotes instead of single quote in markdown...

lshep commented 2 years ago

I apologize for the delay; I was away on holiday. A more detailed review will be done in the next week.

lshep commented 2 years ago

Hello, Please find your initial review below:

Build Report

[ ] Please clean up warnings for imports and documentation.
[ ] Please remove LazyData: true unless necessary.
[ ] Please update your R version to 4.2.
[ ] Please update recommendations on coding practices

Description

[ ] Please remove LazyData: true unless necessary.
[ ] Please update your R version to 4.2.

License

[ ] Clarification: your copyright is for 2019? and HUG? Was this previously released/developed?

NAMESPACE

[ ] Do not selective import and fully import. Choose one or the other for whatever is most appropriate. (ie. GenomicRanges)
[ ] Perhaps you could avoid the warning about replacing 'subtract' function by selectively importing the %>% from magrittr? is full magrittr import necessary?

NEWS

[ ] Currently formatted incorrectly. If you would like to use markdown than please change NEWS to NEWS.md.

inst

[ ] Was there a reason to distinguish between extdata and testdata? If not it is recommended to have one extdata directory.
[ ] Please have a inst/scripts directory that has a file or files that describe how the data in extdata was generated. If keeping separate, also include how the data in testdata was generated. This should include relavent source and reference data. The files can be code, sudo-code, or text.
[ ] Perhaps than inst/scripts is also a better place for the script in inst/bin?

data/man

[ ] The man pages for the data are lacking. Please provide relevant source and reference information. Information on the structure, type, and how one would recreate or create an object of similar type should be included.
[ ] What is the point of defining cntype.xxxx. Users can provide this information in the function call.

man pages

[ ] Please also have a package level man page to direct users to your main function or main vignette. A user should be able to do ?oncoscanR and get information.

vignette

[ ] We recommend using BiocStyle.
[ ] Please at least include a table of contents.
[ ] Please also include in the vignette a motivation for inclusion to Bioconductor and if approrpiate a comparison to similar packages/functions that already exist in Bioconductor. If nothing compare exists that this is fine to state as such.
[ ] Please update installation instructions to show installation from Bioconductor (assuming your package will be accepted and can be installed through normal installation process). BiocManager also installs package from CRAN so you can update/shorten for depdendencies to BiocManager::install(c('GenomicRanges', 'magrittr', 'jsonlite', 'readr')) Also these packages should be installed automatically based on your description file and explicit install should not be necessary.
[ ] Where did oncoscan_na33.cov come from? What is it? How is it generated? Its explained in the man file ?oncoscan_na33.cov but is used and referenced with out any explaintaion in the vignette
[ ] In "Computation of arm-level alteration" you have a typo and need a space after The variable amrleel.lohis

[ ] You don't have to keep repeating the same code chunk

oncoscan.cov <- oncoscan_na33.cov[seqnames(oncoscan_na33.cov) != "21p"]
chas.fn <- system.file("testdata", "triploide_gene_list_full_location.txt", package = "oncoscanR")
segments <- load_chas(chas.fn, oncoscan.cov)

Once that is run, the variables can be used in the later code chunks.

[ ] It seems unnecessary to have the variables for cntype if they are a one vector character vector. Remove and just define as needed. At the very least have getters and setters for defined global variables. Please see how something like AnnotationHub defined global or system variables for use and then has a function in R for viewing or setting any of these internals.

arms.loh <- names(segs.clean[segs.clean$cn.type == "LOH"] %>% armlevel_alt(kit.coverage = oncoscan.cov))

You shouldn't need an internal variable for these?

[ ] In the last section you should show the workflow_oncoscan.run approach since that is the main function run in R.
[ ] There seems to be additional exported functions without detailed explanation of use case in vignette. please expand about intended use cases. If they are not intended for the user than they should not be exported and remain internal functions

Rcode

[ ] See BiocCheck notes about coding practices.
[ ] The functions would benefit from argument validity checks. For instance if an argument is suppose to be a GenomicRanges stopifnot(is(xx, "GRanges")).
[ ] Argument checking and messages should be clear or arguments should be limited. Example if gender can only be 'M' or 'F' right now than when defining the argument function(segments, gender=c('M', 'F') and arg.match. Or be more informative in the message about what the acceptable values are when checking.
[ ] Are the functions in utils designed to be used by the end user? If not do not export.
[ ] Remove unimplemented method load_bed.

Please address the above comments. When ready please be sure to kick off a new build and comment back here with what and how the above concerns are addressed or any clarification points. We appreciate your contribution and look forward to working with you to get it accepted into Bioconductor.

yannchristinat commented 2 years ago

Dear Ishep,

Thank you for the review. I addressed most of your comments and explained the others. The new commit has been pushed to bioconductor.

Hello, Please find your initial review below:

Build Report

[x] Please clean up warnings for imports and documentation.

[ ] Please remove LazyData: true unless necessary.

Cannot load oncoscan_na33.cov.rda if not set.

[x] Please update your R version to 4.2.

[x] Please update recommendations on coding practices

Description

[ ] Please remove LazyData: true unless necessary.

Cannot load oncoscan_na33.cov.rda if not set.

[x] Please update your R version to 4.2.

License

[x] Clarification: your copyright is for 2019? and HUG? Was this previously released/developed?

Copyright set to 2022 with my name and my organisation. (The package is in use internally since 2019 but we finally decided to release it to the community)

NAMESPACE

[x] Do not selective import and fully import. Choose one or the other for whatever is most appropriate. (ie. GenomicRanges)

[x] Perhaps you could avoid the warning about replacing 'subtract' function by selectively importing the %>% from magrittr? is full magrittr import necessary?

Thanks for the tip!

NEWS

[x] Currently formatted incorrectly. If you would like to use markdown than please change NEWS to NEWS.md.

inst

[x] Was there a reason to distinguish between extdata and testdata? If not it is recommended to have one extdata directory.

[x] Please have a inst/scripts directory that has a file or files that describe how the data in extdata was generated. If keeping separate, also include how the data in testdata was generated. This should include relavent source and reference data. The files can be code, sudo-code, or text.

[x] Perhaps than inst/scripts is also a better place for the script in inst/bin?

Most files have been moved to ext data. Initially, I did not want the user to have access to these files (hence the testdata folder). But with the extension of the vignette they turn out quite useful for demonstrating the package usage. Files truly only used for testing have been moved to 'tests/testdata'.

data/man

[ ] The man pages for the data are lacking. Please provide relevant source and reference information. Information on the structure, type, and how one would recreate or create an object of similar type should be included.

Which data are you talking about? The two Rda files and how they were generated is described in R/data.R and I added a README.md in the inst/extdata folder.

[x] What is the point of defining cntype.xxxx. Users can provide this information in the function call.

Redesigned the use of theses variables. Created an internal cntypes list instead of these.

man pages

[x] Please also have a package level man page to direct users to your main function or main vignette. A user should be able to do ?oncoscanR and get information.

vignette

[x] We recommend using BiocStyle.

[x] Please at least include a table of contents.

[x] Please also include in the vignette a motivation for inclusion to Bioconductor and if approrpiate a comparison to similar packages/functions that already exist in Bioconductor. If nothing compare exists that this is fine to state as such.

[x] Please update installation instructions to show installation from Bioconductor (assuming your package will be accepted and can be installed through normal installation process). BiocManager also installs package from CRAN so you can update/shorten for depdendencies to BiocManager::install(c('GenomicRanges', 'magrittr', 'jsonlite', 'readr')) Also these packages should be installed automatically based on your description file and explicit install should not be necessary.

[x] Where did oncoscan_na33.cov come from? What is it? How is it generated? Its explained in the man file ?oncoscan_na33.cov but is used and referenced with out any explaintaion in the vignette

Added a paragraph on this topics.

[x] In "Computation of arm-level alteration" you have a typo and need a space after The variable amrleel.lohis

[x] You don't have to keep repeating the same code chunk
oncoscan.cov <- oncoscan_na33.cov[seqnames(oncoscan_na33.cov) != "21p"]
chas.fn <- system.file("testdata", "triploide_gene_list_full_location.txt", package = "oncoscanR")
segments <- load_chas(chas.fn, oncoscan.cov)
Once that is run, the variables can be used in the later code chunks.

[x] It seems unnecessary to have the variables for cntype if they are a one vector character vector. Remove and just define as needed. At the very least have getters and setters for defined global variables. Please see how something like AnnotationHub defined global or system variables for use and then has a function in R for viewing or setting any of these internals.

arms.loh <- names(segs.clean[segs.clean$cn.type == "LOH"] %>% armlevel_alt(kit.coverage = oncoscan.cov))

You shouldn't need an internal variable for these?

Created new functions to obtain segments by CN type:

get_amp_segments
get_gain_segments
get_hetloss_segments
get_loss_segments
get_loh_segments

[x] In the last section you should show the workflow_oncoscan.run approach since that is the main function run in R.

[x] There seems to be additional exported functions without detailed explanation of use case in vignette. please expand about intended use cases. If they are not intended for the user than they should not be exported and remain internal functions

Added a use case for score_avgcn, score_estwgd, score_mbalt.

Rcode

[x] See BiocCheck notes about coding practices.

All lines are below 80 characters and long functions have been split. The largest function is now only 65 lines. On a few lines, the indent is not a multiple of 4 but that is for a better readability.

[x] The functions would benefit from argument validity checks. For instance if an argument is suppose to be a GenomicRanges stopifnot(is(xx, "GRanges")).

[x] Argument checking and messages should be clear or arguments should be limited. Example if gender can only be 'M' or 'F' right now than when defining the argument function(segments, gender=c('M', 'F') and arg.match. Or be more informative in the message about what the acceptable values are when checking.

Removed the gender argument as it did not add any information.

[x] Are the functions in utils designed to be used by the end user? If not do not export.

Made the utils functions private.

[x] Remove unimplemented method load_bed.

yannchristinat commented 2 years ago

Dear @lshep, Is there anything that have to do and did not realize?

lshep commented 2 years ago

Thank you. I will try and re-review the package in the next day or two.

lshep commented 2 years ago

Can you please bump your version number and push to git.bioconductor.org so a new build report is triggered

lshep commented 2 years ago

And the NEWS file still needs to be renamed from NEW to NEWS.md

bioc-issue-bot commented 2 years ago

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

yannchristinat commented 2 years ago

And the NEWS file still needs to be renamed from NEW to NEWS.md

Done

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.

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/yannchristinat.keys is not empty), then no further steps are required. Otherwise, do the following:

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

https://bioconductor.org/packages/oncoscanR

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.

Bioconductor / Contributions

oncoscanR #2644