Bioconductor / Contributions

Contribute Packages to Bioconductor
135 stars 33 forks source link

SparseSignaturesPlus #2798

Closed danro9685 closed 2 years ago

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

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: SparseSignaturesPlus
Version: 0.99.1
Date: 2022-10-03
Title: SparseSignaturesPlus
Authors@R: c(
    person("Daniele", "Ramazzotti", role=c("cre","aut"),email="daniele.ramazzotti@unimib.it",
      comment = c(ORCID = "0000-0002-6087-2666")),
    person("Luca", "De Sano", role=c("aut"), email="luca.desano@gmail.com",
      comment = c(ORCID = "0000-0002-9618-3774")))
Maintainer: Luca De Sano <luca.desano@gmail.com>
Depends:
    R (>= 4.1.0),
    NMF
Imports:
    Biostrings,
    BSgenome,
    BSgenome.Hsapiens.1000genomes.hs37d5,
    data.table,
    GenomeInfoDb,
    GenomicRanges,
    glmnet,
    ggplot2,
    gridExtra,
    IRanges,
    lsa,
    nnls,
    reshape2
Suggests:
    BiocGenerics,
    BiocStyle,
    testthat,
    knitr,
Name:
    An R package for the the efficient extraction and assignment of mutational signatures from cancer genomes
Description:
    Cancer is a genetic disease caused by somatic mutations in genes controlling key biological functions such as cellular growth and division. Such mutations may arise both through cell-intrinsic and exogenous processes, generating characteristic mutational patterns over the genome named mutational signatures. The study of mutational signatures have become a standard component of modern genomics studies, since it can reveal which (environmental and endogenous) mutagenic processes are active in a tumor, and may highlight markers for therapeutic response. Mutational signatures computational analysis presents many pitfalls. First, the task of determining the number of signatures is very complex and depends on heuristics. Second, several signatures have no clear etiology, casting doubt on them being computational artifacts rather than due to mutagenic processes. Last, approaches for signatures assignment are greatly influenced by the set of signatures used for the analysis. To overcome these limitations, we developed SparseSignaturesPlus, a framework that allows the efficient extraction and assignment of mutational signatures.
Encoding: UTF-8
License: file LICENSE
URL: https://github.com/danro9685/SparseSignaturesPlus
BugReports: https://github.com/danro9685/SparseSignaturesPlus
biocViews: BiomedicalInformatics, 
 SomaticMutation
RoxygenNote: 7.2.1
VignetteBuilder: knitr
vjcitn commented 2 years ago

Thanks for this submission. There are already a few packages in Bioconductor addressing mutational signatures -- MutationalPatterns, SomaticSignatures, musicatk. Can you give some background on how your package adds to the toolset for analysts in this domain? Are any of the data structures in those packages -- or other Bioconductor data structures -- worth reusing in your package?

danro9685 commented 2 years ago

Dear @vjcitn,

This package implements a new algorithm for mutational signatures analysis, which is not currently included in any of the packages you have mentioned. We will clarify this and also check for other Bioconductor packages that might be worth reusing.

Thank you.

Best, Daniele

danro9685 commented 2 years ago

Dear @vjcitn,

We have updated the documentation as suggested. Please let us know if you have additional pre-review suggestions.

Thanks, Daniele

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

luca-dex commented 2 years ago

Dear @vjcitn I'm the maintainer of the package. I already added the bioconductor remotes to our package and we're trying to push some changes but, at the moment, SparseSignaturePlus is not among the packages to which I access on git.bioconductor.org so my SSH key is not accepted.

$ git fetch --all
Fetching origin
Fetching upstream
FATAL: R any packages/SparseSignaturePlus l.desano DENIED by fallthru
(or you mis-spelled the reponame)
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
error: could not fetch upstream

Could you please help us in sorting out this issue?

Thanks

lshep commented 2 years ago

@luca-dex Bioconductor by default (and encourages) only one active maintainer. Daniele was given immediate access as that was who was listed as the maintainer in the DESCRIPTION. I have made the necessary adjustments on our end and you should have access now.

luca-dex commented 2 years ago

Thank @lshep , I'll wait to have SpareSignaturePlus in the list of package on BiocCredentials after it will be hopefully accepted.

@vjcitn we did the necessary updates on our github, so the issue that were highlighed by BiocCheck, should have been solved

Thanks for the support

lshep commented 2 years ago

@luca-dex sorry I forgot to push the changes up. You should have access now.

nilseling commented 2 years ago

Dear @luca-dex and @danro9685 ,

thanks for submitting the SparseSignaturesPlus package. I had a first look at the package and there are some things that need to be addressed before acceptance (see below). You are currently maintaining the SparseSignatures package and I was wondering about the motivation to create a new package rather than integrating the described functions into the existing package?

Please provide a point-by-point response to the checklist below and let me know if you have questions.

General package development

* checking R code for possible problems ... NOTE
get.MNV.counts: no visible global function definition for ‘.’
get.SBS.counts: no visible global function definition for ‘.’
groups.CN.plot: no visible binding for global variable ‘value’
groups.CN.plot: no visible binding for global variable ‘alt’
groups.CX.plot: no visible binding for global variable ‘value’
groups.MNV.plot: no visible binding for global variable ‘Context’
groups.MNV.plot: no visible binding for global variable ‘value’
groups.MNV.plot: no visible binding for global variable ‘alt’
groups.SBS.plot: no visible binding for global variable ‘Context’
groups.SBS.plot: no visible binding for global variable ‘value’
groups.SBS.plot: no visible binding for global variable ‘alt’
patients.CN.plot: no visible binding for global variable ‘alt’
patients.CN.plot: no visible binding for global variable ‘patient’
patients.CN.plot: no visible binding for global variable ‘value’
patients.CX.plot: no visible binding for global variable ‘patient’
patients.CX.plot: no visible binding for global variable ‘value’
patients.MNV.plot: no visible binding for global variable ‘Context’
patients.MNV.plot: no visible binding for global variable ‘alt’
patients.MNV.plot: no visible binding for global variable ‘patient’
patients.MNV.plot: no visible binding for global variable ‘value’
patients.SBS.plot: no visible binding for global variable ‘Context’
patients.SBS.plot: no visible binding for global variable ‘alt’
patients.SBS.plot: no visible binding for global variable ‘patient’
patients.SBS.plot: no visible binding for global variable ‘value’
signatures.CN.plot: no visible binding for global variable ‘alt’
signatures.CN.plot: no visible binding for global variable ‘value’
signatures.CX.plot: no visible binding for global variable ‘value’
signatures.MNV.plot: no visible binding for global variable ‘Context’
signatures.MNV.plot: no visible binding for global variable ‘alt’
signatures.MNV.plot: no visible binding for global variable ‘value’
signatures.SBS.plot: no visible binding for global variable ‘Context’
signatures.SBS.plot: no visible binding for global variable ‘alt’
signatures.SBS.plot: no visible binding for global variable ‘value’

For the plotting functions you could use aes_string() instead of aes() to resolve the issue.

The DESCRIPTION file

The NAMESPACE file

The NEWS file

Package data

Documentation

Vignette

Man pages

Unit tests

SparseSignaturesPlus Coverage: 0.00%
R/data.import.R: 0.00%
R/mutational.signatures.discovery.R: 0.00%
R/mutational.signatures.significance.R: 0.00%
R/visualization.R: 0.00%

R code

* NOTE: Avoid 1:...; use seq_len() or seq_along()
* NOTE: Avoid 'cat' and 'print' outside of 'show' methods
* NOTE: Avoid using '=' for assignment and use '<-' instead
* WARNING: Avoid class membership checks with class() / is() and == / !=; Use is(x, 'class') for S4 classes
* WARNING: Remove set.seed usage (found 4 times)
* WARNING: Avoid the use of 'library' or 'require' in R code
res <- cv.glmnet(cbind(t(curr_beta_values),matrix(rep(0,ncol(curr_beta_values)),ncol=1)),as.vector(x[j,]),type.measure="mse",nfolds=10,nlambda=10,family="gaussian",lower.limits=0.00)
danro9685 commented 2 years ago

Dear @nilseling,

Thanks for your very detailed review of our package.

We have just pushed on the GitHub page of the tool changes addressing: (i) all your points for "General package development", "The DESCRIPTION file" and "The NAMESPACE file" and (ii) most of your points for "Documentation" and "R code". We are taking care of the remaining things as soon as possible.

In the mean time, we wanted to ask your advice regarding the package name. This tool (SparseSignaturesPlus) implements a new algorithm for mutational signatures analysis. The related paper is in preparation and will be referred in the package as soon as it is ready, but we don't think this will happen by the next Bioconductor release. To this end, besides providing details on the algorithm, do you think it would make sense to also change the package name? Maybe this would reduce the confusion regarding the already published SparseSignatures algorithm, from which SparseSignaturesPlus takes some inspiration, but the two methods are different.

Please let us know your thoughts. Thanks, Daniele and @luca-dex

nilseling commented 2 years ago

Hi @luca-dex and @danro9685

I would then propose to change the name of the package to avoid confusion. If you agree with this I will close this issue and wait for you to open a new issue with the new name (see package name for guidance). We will then fast track the issue and I will continue with the review where we left off here. Does that sound alright?

danro9685 commented 2 years ago

Dear @nilseling,

This sounds great, as new name, what about: RESOLVE (Robust EStimation Of mutationaL signatures Via rEgularization). If this is good, we can proceed as you suggested.

Thanks, Daniele and Luca

nilseling commented 2 years ago

Sounds good. I had a quick check with available("RESOLVE") and the name seems to be alright. I will close this issue now - please open a new issue using RESOLVE

danro9685 commented 2 years ago

Dear @nilseling I have opened the issue for the new package. Please add @luca-dex as primary maintainer of the package. Thanks!