search

Bioconductor / Contributions

Contribute Packages to Bioconductor
134 stars 33 forks source link

(inactive) NucDyn #1092

Closed dcbuitrago closed 5 years ago

dcbuitrago commented 5 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 help with submitting your package, please subscribe and post questions to the bioc-devel mailing list.

bioc-issue-bot commented 5 years ago

Hi @dcbuitrago

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: NucDyn
Type: Package
Title: Nucleosome Dynamics
Version: 0.99.0
Date: 2018-02-13
Author: Ricard Illa, Diana Buitrago
Maintainer: Diana Buitrago <diana.buitrago@irbbarcelona.org>
Description: Nucleosome occupancy dynamic comparison between two reference states.
License: file LICENSE
Depends: R (>= 3.5), methods
Imports: GenomeInfoDb, GenomicRanges, S4Vectors, BiocGenerics, IRanges, nucleR, parallel, plyr, doParallel, Rcpp, graphics, stats
Suggests: BiocStyle, knitr, rmarkdown
LazyLoad: yes
LinkingTo: Rcpp
VignetteBuilder: knitr
RoxygenNote: 6.1.1
Roxygen: list(markdown = TRUE)
biocViews: Sequencing, Genetics, MNaseSeq
Encoding: UTF-8 
URL: http://mmb.irbbarcelona.org/NucleosomeDynamics/help/method#nd

Add SSH keys to your GitHub account. SSH keys will are used to control access to accepted Bioconductor packages. See these instructions to add SSH keys to your GitHub account.

bioc-issue-bot commented 5 years ago

A reviewer has been assigned to your package. Learn what to expect during the review process.

IMPORTANT: Please read the instructions for setting up a push hook on your repository, or further changes to your repository will NOT trigger a new build.

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

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

e12c809 From Version: 0.99.0 to Version: 0.99.1

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

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

6a326ae From Version: 0.99.1 to Version: 0.99.2

bioc-issue-bot commented 5 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: "TIMEOUT". 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.

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

429bda6 From Version: 0.99.2 to Version: 0.99.3

bioc-issue-bot commented 5 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, skipped, 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.

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

c25b004 From Version: 0.99.3 to Version: 0.99.4

bioc-issue-bot commented 5 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: "skipped, ERROR, 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.

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

d7b5e09 from Version: 0.99.4 to Version 0.99.5

bioc-issue-bot commented 5 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: "skipped, 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.

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

885b2cf From Version: 0.99.5 to Version: 0.99.6

bioc-issue-bot commented 5 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, skipped, 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.

hpages commented 5 years ago

Hi @dcbuitrago ,

Thanks for submitting the NucDyn package and sorry for the long delay in getting back to you.

The package will need some improvements. In particular, its man pages need attention. Generally speaking, attention to details and more precise/accurate language is expected. See details below.

Best, H.

  1. Remove all files from the inst/doc/ folder. These files are stale and do not belong to the package source tree. The correct files will be automatically generated and added to the package source tarball by R CMD build. Note that other packages from your lab (chroGPS and htSeqTools) have a similar problem. Also chroGPS/inst/doc contains a NEWS file but this is not the correct location for this file (see the "Writing R Extensions" manual for the details).

  2. Despite what their man pages say, datasets readsG2_chrII and readsM_chrII are not RangedData objects.

  3. RangedData objects have been superseeded by GRanges objects and their use discouraged for years. Furthermore, they were deprecated in BioC 3.9. See ?RangedData. Please remove any code related to RangedData objects as well as any reference to them.

  4. Because the main input and output objects are GRanges and GRangesList objects, the GenomicRanges package should be in Depends rather than Imports. This way the NucDyn user can immediately operate on these objects. Right now very basic operations on these objects don't work out-of-the-box:

    library(NucDyn)
data(readsG2_chrII)
seqnames(readsG2_chrII)
# Error in seqnames(readsG2_chrII) : could not find function "seqnames"
ranges(readsG2_chrII)
# Error in ranges(readsG2_chrII) : could not find function "ranges"
strand(readsG2_chrII)
# Error in strand(readsG2_chrII) : could not find function "strand"

  5. You need to be more precise/careful with how you spell things. For example in the man page for nucleosomeDynamics you write:

    The aim of NucleosomeDynamics is to infer "movement" (with
direction and magnitude) of the reads between two reference
nucleosome maps.

    It's not clear what "NucleosomeDynamics" refers to exactly: is it the field (in which case it should be "Nucleosome Dynamics"), or is it your package (in which case it should be "NucDyn"), or is it the nucleosomeDynamics() function? The answer may be be obvious, or maybe it's not. In any case, the reader shouldn't have to guess.

    Note that the Description section of the same man page starts with a similar problem:

    This is the main function in nucleosomeDynamics.

    I guess you mean that nucleosomeDynamics is the main function in the NucDyn package. The entire documentation suffers from this lack of precision in the language.

  6. Not sure why you have so many calls in the example sections of your man pages wrapped in \donttest statements. This is not a good idea and should be avoided.

  7. nucleosomeDynamics() should probably handle the following edge case more gracefully:

    dyn <- nucleosomeDynamics(setA=GRanges("chr1", IRanges(1:5, 10)),
                          setB=GRanges("chr2", IRanges(11:15, 20)))
# Starting chr1
# Error in (function (classes, fdef, mtable)  : 
#   unable to find an inherited method for function ‘ranges’ for signature ‘"NULL"’

  8. In this example the range in setB disappears i.e. is not reported in the output:

    library(GenomicRanges)
setA <- GRanges("chr1:10-22")
setB <- GRanges("chr1:1000-2001")
dyn <- nucleosomeDynamics(setA, setB)
set.b(dyn)$originals
# GRanges object with 0 ranges and 1 metadata column:
#    seqnames    ranges strand |  type
#       <Rle> <IRanges>  <Rle> | <Rle>
#   -------
#   seqinfo: no sequences

    That doesn't seem right. More generally speaking, if the originals group contains "all reads present for that set before the analysis" as stated in the man page for the NucDyn class, shouldn't length(set.a(dyn)$originals) and length(set.b(dyn)$originals) always be equal to length(setA) and length(setB), respectively? That doesn't seem to be the case for the example in ?nucleosomeDynamics:

    length(readsG2_chrII)
# [1] 55416
length(readsM_chrII)
# [1] 27564
dyn <- nucleosomeDynamics(setA=readsG2_chrII, setB=readsM_chrII)
length(set.a(dyn)$originals)
# [1] 49501
length(set.b(dyn)$originals)
# [1] 26484

  9. The small.shifts group is not documented.

  10. The man page for findHotspots() provides no explanation of what hot spots are or what the function does. Also the Value section seems out-of-sync with what the function actually returns (when running the example provided in the man page, I get a data.frame with 7 columns, not 4, and none of them is named chrom or coord).

  11. Similarly the man page for findPVals() is too minimalist. The user needs to know exactly what the function is doing. Right now the findHotspots() and findPVals() functions are black boxes.

  12. Same problem with the man pages for datasets sample_chrII and nuc_chrII.

  13. In addition to being too minimalist, the man page for sample_chrII is also confusing: the Description section seems to describe 2 datasets:

    Data from a MNaseSeq experiment in yeast in the following mitosis
phases: G2 and M

Data from a MNaseSeq experiment in S. cerevisiae (sacCer3) in the
following cell cycle phases: G2 and M

    but the 2 descriptions are actually the same (phrased slightly differently). Also the Usage section actually loads twice the same dataset. Are these repetitions intended? Are there actually 2 datasets? Where are they? What's the difference between the 2 datasets? The Value section only says that the dataset is a list but it should provide more details e.g. what's in the list? is the list named? etc... Also why is one list element an integer vector and the other one a numeric vector? Couldn't they both be integer vectors?

  14. You have man pages for things that the user doesn't have access to. For example, the manual documents the applyThreshold and plotDynamics functions but the package doesn't provide such functions. This can only confuse the end-user. (It could be that these are internal functions but this is not relevant from an end-user point of view.)

  15. The vignette says:

    BAM files containing aligened reads can by imported in R using
scanBam function from Rsamtools package and then transformed to
GRanges format.

    Importing aligned reads from a BAM file into a GRanges object is not trivial, especially if the aligned reads have non-simple CIGARs, in which case the CIGAR string has to be accounted for in order to get the correct end position of the read. People trying to import aligned reads with scanBam() generally don't do that so they end up with the wrong genomic ranges. This is why most users should preferrably import the reads with the readGAlignments() function from the GenomicAlignments package, and then use granges() to extract the genomic ranges from the GAlignments object returned by readGAlignments().

  16. The show method for NucDyn objects could be improved. One problem is that it prints lines that are too long. As a consequence they don't fit in the PDF version of the vignette (see output of print(dyn)). More generally speaking, a show method should honor the width global option i.e. not print lines that are longer than the value of this option. BTW have you considered printing the group sizes in 2 columns, one for "Set a" and one for "Set b"? Wouldn't that be a more sensible layout? Note that this would solve the long line problem.

  17. Typos in vignettes:

    • "rearrengements" (should be "rearrangements")
    • "aligened" (should be "aligned")
    • and maybe more... (please check the spelling)
lshep commented 5 years ago

We really like to see progress on packages in submission. Do you plan on submitting an updated version of the package soon?

dcbuitrago commented 5 years ago

We are currently working on the suggested revisions. We have now worked on all the modifications regarding the documentation but the changes regarding the use of RangedData objects required more extensive changes and we are making sure all functions in the package work properly without this object type. We plan to submit the update version by the end of the month.

I apologise for the late reply,

Best regards,

Diana Buitrago

On Thu, Jul 18, 2019 at 2:09 PM lshep notifications@github.com wrote:

We really like to see progress on packages in submission. Do you plan on submitting an updated version of the package soon?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Bioconductor/Contributions/issues/1092?email_source=notifications&email_token=ALUROV2VEI5QUN4PEUOAPBTQABMOZA5CNFSM4HFYVE22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2IIQ6I#issuecomment-512788601, or mute the thread https://github.com/notifications/unsubscribe-auth/ALUROVYGPGWICHXMTMGIFITQABMOZANCNFSM4HFYVE2Q .

-- Molecular Modeling and Bioinformatics Group Institute for Research in Biomedicine (IRB Barcelona) C/ Baldiri Reixac 10 08028 Barcelona - Spain

hpages commented 5 years ago

Thanks for the update @dcbuitrago

lshep commented 5 years ago

Is there a status update on this package? We like to see changes within 2 weeks - for more extensive changes 1-2 months. There has been no activity since Apr 17th. May we expect and update soon? If you need more time to work on the package we can close the issue and you can re-open when you are ready to commit changes.

dcbuitrago commented 5 years ago

Thank you for contacting us. In that case it would be better to close the issue and we will re-open it once we have the changes ready

Regards,

On Thu, Sep 5, 2019 at 1:30 PM lshep notifications@github.com wrote:

Is there a status update on this package? We like to see changes within 2 weeks - for more extensive changes 1-2 months. There has been no activity since Apr 17th. May we expect and update soon? If you need more time to work on the package we can close the issue and you can re-open when you are ready to commit changes.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Bioconductor/Contributions/issues/1092?email_source=notifications&email_token=ALUROV7B5LGKI7DCUBWDUYDQIDUVBA5CNFSM4HFYVE22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD56YSPI#issuecomment-528320829, or mute the thread https://github.com/notifications/unsubscribe-auth/ALUROV7FSZ7BMZAKY6WIIOTQIDUVBANCNFSM4HFYVE2Q .

-- Molecular Modeling and Bioinformatics Group Institute for Research in Biomedicine (IRB Barcelona) C/ Baldiri Reixac 10 08028 Barcelona - Spain

lshep commented 5 years ago

@dcbuitrago No problem at all. Please just make a comment back here on this issue to have us reopen when ready.
Cheers,

bioc-issue-bot commented 5 years ago

This issue is being closed because there has been no progress for an extended period of time. You may reopen the issue when you have the time to actively participate in the review / submission process. Please also keep in mind that a package accepted to Bioconductor requires a commitment on your part to ongoing maintenance.

Thank you for interest in Bioconductor.