Bioconductor / Contributions

Contribute Packages to Bioconductor
134 stars 33 forks source link

ddPCRclust #575

Closed bgbrink closed 6 years ago

bgbrink commented 6 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 6 years ago

Hi @bgbrink

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: ddPCRclust
Title: Clustering algorithm for ddPCR data
Version: 0.99.0
Date: 2017-11-11
Authors@R: c(person("Benedikt G.", "Brink", email = "bbrink@cebitec.uni-bielefeld.de", role = c("aut", "cre")), person("Justin", "Meskas", email = "jmeskas@bccrc.ca", role = "ctb"), person("Ryan R.", "Brinkman", email = "rbrinkman@bccrc.ca", role = "ctb"))
Description: The ddPCRclust algorithm can automatically quantify the events of ddPCR reaction with up to four markers. In order to determine the correct droplet count for each marker, it is crucial to both identify all clusters and label them correctly based on their position. For more information on what data can be analyzed and how a template needs to be formatted, please check the project repository on github.
LazyData: true
Depends:
    R (>= 3.4)
Imports: 
    plotrix,
    clue,
    parallel,
    ggplot2,
    openxlsx,
    R.utils,
    flowDensity (>= 1.13.3),
    SamSPECTRAL,
    flowPeaks,
    BiocStyle
Remotes: 
    bioc::flowDensity, 
    bioc::SamSPECTRAL, 
    bioc::flowPeaks,
    bioc::BiocStyle
Collate: 
    'cluster_functions.R'
    'functions.R'
    'ddPCRclust.R'
License: file LICENSE
URL: https://github.com/bgbrink/ddPCRclust
biocViews:
    ddPCR,
    Clustering
RoxygenNote: 6.0.1
mtmorgan commented 6 years ago

before adding your package to the review queue, please update the vignette to have fully evaluated code chunks <<>>= rather than verbatim 'pictures of code'. Remember the package guidelines about time and space limitations.

bgbrink commented 6 years ago

I pushed the requested changes to the repo 5 days ago. Do you get notified directly via the webhook, or should I adress this and future comments throughout the review process here?

bioc-issue-bot commented 6 years ago

Your package has been approved for building. Your package is now submitted to our queue.

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.

mtmorgan commented 6 years ago

The issue hadn't been accepted to the moderation queue so the web hook was not active; thanks for your initial change, a reviewer has been assigned.

bioc-issue-bot commented 6 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 following build report for more details:

http://bioconductor.org/spb_reports/ddPCRclust_buildreport_20171218061842.html

bioc-issue-bot commented 6 years ago

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

ee73e0b DESCRIPTION update

bioc-issue-bot commented 6 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 following build report for more details:

http://bioconductor.org/spb_reports/ddPCRclust_buildreport_20171218065258.html

bgbrink commented 6 years ago

The build command says "ERROR: dependency flowDensity is not available for package ddPCRclust" on Mac OSX. Is this a problem with the build system?

LiNk-NY commented 6 years ago

Hi Benedikt, @bgbrink This is a problem with the available Mac binaries for OSX. Please ignore the error. It should go away once the R-development binaries are posted by CRAN.

Regards, Marcel

bioc-issue-bot commented 6 years ago

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

f974761 Merge pull request #1 from bgbrink/master Pulling 0456699 Merge pull request #2 from bgbrink/master Pulling... f2c7ad9 Justin changed the vignette a little. 025e742 Merge pull request #3 from jmeskas/master Justin ...

bioc-issue-bot commented 6 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 following build report for more details:

http://bioconductor.org/spb_reports/ddPCRclust_buildreport_20171220151840.html

bioc-issue-bot commented 6 years ago

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

d265d52 v0.99.4 several improvements and bugfixes

bioc-issue-bot commented 6 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 6 years ago

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

af803d7 v0.99.5 removed inst/doc

bioc-issue-bot commented 6 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 6 years ago

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

7527fb6 v0.99.6 bugfix

bioc-issue-bot commented 6 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 6 years ago

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

e16a887 v0.99.7 marginal change

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

LiNk-NY commented 6 years ago

Hi Benedikt, @bgbrink

Please remove the Remotes field from your DESCRIPTION file. It is not necessary. All of these Bioconductor packages are included in the BBS. You may even be installing out of date packages that way.

Also, please move BiocStyle to the Suggests field.

Regards, Marcel

bioc-issue-bot commented 6 years ago

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

4ad1eb3 v0.99.8 removed Remotes from DESCRIPTION

bioc-issue-bot commented 6 years ago

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

2c54029 v0.99.9 moved BiocStyle from Imports to Suggests

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

LiNk-NY commented 6 years ago

Hi Benedikt, @bgbrink

I took a quick look at the package and I had a few issues with its implementation.

Best regards, Marcel

bgbrink commented 6 years ago

Dear Marcel, @LiNk-NY

thank you for your input. I addressed all your points except the third one. May I ask what the reasoning behind avoiding @ is?

I am using the @exprs tag to override the expression data of a flowFrame with data from a ddPCR experiment. Simply using the designated function flowCore::exprs() produces an error, because the data don't match. Thus, avoiding these checks is intentional and crucial to our ddPCRclust package, since we want to apply some methods from flow cytometry to ddPCR. I would be very grateful for any advice on how to keep the core functionality of our package intact and comply with Bioconductor's coding standards at the same time.

Best regards, Benedikt

LiNk-NY commented 6 years ago

Hi Benedikt, @bgbrink

As in the link provided, it separates the implementation from the interface. Our coding standards require that you provide a clean user interface in the package.

But based on your description, it seems like it is an issue for the flowCore maintainers. I would create a GitHub issue about it if it is not giving you what you expect.

Regards, Marcel

bgbrink commented 6 years ago

Edit: Never mind.

bioc-issue-bot commented 6 years ago

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

2c7e4ff v0.99.10 Bioconductor coding standard compliance

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

bgbrink commented 6 years ago

Hi Marcel, @LiNk-NY

I was finally able to follow all your previous suggestions, thank you. However, moving BiocStyle to the Suggests field now produces an error when running R CMD check. The NAMESPACE was generated automatically by roxygen2.

LiNk-NY commented 6 years ago

Hi Benedikt, @bgbrink

You have a stray import directive in your files. See: R/ddPCRclust.R:27.

Regards, Marcel

bgbrink commented 6 years ago

Hi Marcel, @LiNk-NY

...yes, sorry. Can I keep the import directives for the functions that make heavy use of the respective packages (namely flowDensity, SamSPECTRAL, flowPeaks, and clue) or should all imports be removed regardless?

LiNk-NY commented 6 years ago

Hi Benedikt, @bgbrink

In general, you want to import packages whose functions you are using within your own functions. In this case, you're not using any of the BiocStyle functions within your functions so it should be removed from the imports.

Line 27 should read:

@import plotrix

Preferably, you want to selectively import functions rather than whole packages to avoid name collisions. For example, @importFrom plotrix clplot.

Regards, Marcel

bioc-issue-bot commented 6 years ago

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

1342f12 v0.99.11 fixed imports

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

LiNk-NY commented 6 years ago

Hi Benedikt, @bgbrink Thank you for submitting to Bioconductor. ddPCRclust provides an algorithm for identifying clusters in ddPCR reaction data.

Although it uses other Bioconductor packages, ddPCRclust does not seem to use existing data structures already available. Please find a data structure that fits the data, namely from flow* packages.

Please see the review below.

Best regards, Marcel


ddPCRclust #575

DESCRIPTION

NAMESPACE

R

Minor:

vignettes

LICENSE

bgbrink commented 6 years ago

Hi Marcel, @LiNk-NY thank you for your feedback, it is much appreciated. I am working my way through your comments and a few questions came up.

Please use a data structure that will be an input to ddPCRclust, using files is not very extendable.

I have seen some packages that take a string representing the location of a file as in input before and I always found that very practical for the end user. Thus, I implemented ddPCRclust the same way. If I move the file-reading part of the main function into a different function, that may or may not be used by the end user, while the main function takes the resulting data frame as an input, would that work for you?

Consider using a more appropriate structure for your algorithm with validity checks. A list output will be prone to errors.

I do not understand this to be honest. Could you give me an example for a more appropriate structure?

Does it have to be an academic only license?

We have reasons to leave the licence how it is now, is this going to be a problem with making ddPCRclust available on Bioconductor?

Best regards, Benedikt

LiNk-NY commented 6 years ago

Hi Benedikt, @bgbrink

A. Functions should perform a single task and not try to do too much. When you introduce a file argument, you limit the way the data can be read in. In cases where different file types arise for similar data, the function would suffer in portability.

When using a data structure, such as data.frame, as input, the user can read in and manipulate the data before passing it on to the algorithm.

B. My point about the data structure is that you may want to use a tried and tested data class such as SummarizedExperiment to represent your data. This class would come with validity checks and is more robust than a simple list.

C. With regards to the license, I'm not sure but I will ask Martin @mtmorgan about this.

Regards, Marcel

LiNk-NY commented 6 years ago

Hi Benedikt, @bgbrink

After consulting with Martin @mtmorgan, I was informed that we are unable to accept a package with an academic only license. If you are unable to change the license restriction, we will be forced to close the issue.

Thank you for your attention.

Regards, Marcel

bgbrink commented 6 years ago

Hi Marcel, @LiNk-NY

we will change the license to artistic-2.0.

I have been implementing most of your suggested changes over the weekend, however I still have a question regarding the previously discussed list output. Changing the output to different data class will require extensive changes not only to ddPCRclust, but also to the accompanying shiny app ddPCRvis (https://github.com/bgbrink/ddPCRvis). I estimate that a significant amount of time would be necessary to implement and test these changes. Since I am only able to work on this project in my free time, I am not sure that I can provide what you ask at the moment. However, I would be happy to keep it in mind for possible future versions of ddPCRclust and ddPCRvis. Alternatively, maybe you can point me in a different direction whith faster to implement changes. I really hope we can find a solution to make ddPCRclust available on Bioconductor while meeting the high standard of Bioconductor packages.

bioc-issue-bot commented 6 years ago

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

5c386a6 v0.9.12 input file reading separated from main fun...

bioc-issue-bot commented 6 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, WARNINGS, TIMEOUT, 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.

bgbrink commented 6 years ago

Hi Marcel, @LiNk-NY

in the new Version most of your requests have been implemented. However, I did not remove the export* functions, since I use them in the shiny app and I think some users might find them useful. Furthermore, as aforementioned, I did not (yet) change the output of the functions.

I did not get an error on my Linux machine when building and checking the package. I will look into that on the weekend.

Lastly, I tried using the formatR package to format the code in order to comply with the 80 columns limit. It seems this did not work very well, since BiocCheck sill reports about 20% of lines exceeding this limit. Is it really expected to manually go through thousands of lines of code and apply this limit by hand? May I ask what the motivation behind this is?

Best regards, Benedikt

LiNk-NY commented 6 years ago

Hi Benedikt, @bgbrink Thank you for making those changes. It would be good for your package to be able to benefit from the established classes in Bioconductor.

List outputs are not very structured and can be prone to errors. They also don't have established data validity methods. Please consider using a Bioconductor class for your results.

It seems to me like these functions should go with the shiny app rather than here.

Readable code is easier to review and maintain. We don't strictly enforce the 80 column width limit but it reflects on the quality of the coding to some extent.

Best regards, Marcel

bioc-issue-bot commented 6 years ago

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

a584a67 v0.9.13 vignette update

bioc-issue-bot commented 6 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, TIMEOUT, 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.

bgbrink commented 6 years ago

Hi Marcel, @LiNk-NY I cannot reproduce the timeout issue on the machines that are available to me. Do you have any idea what might be the cause here?

Regarding the requested changes, I understand and agree that it would be good to benefit from established Bioconductor classes. However, I would like to ask you to consider that the package is working perfectly fine as it is and the requested changes would require a significant amount of time. I am finalising the revision for the manuscript about ddPCRclust, which has been submitted to Bioinformatics, and I would very much like to be able to include the link to Bioconductor in the final version of the paper. Could you point out to me which changes absolutely have to be done before the package can be accepted on Bioconductor, so I can prioritise appropriately in order to have a chance to make it in time?

LiNk-NY commented 6 years ago

Hi Benedikt, @bgbrink

Thanks for making those changes.

It is understandable that making the requested changes will take a significant amount of time. In the time being, your package will be accepted. Hopefully, you consider making these changes as you continue to maintain and develop your package.

Thank you for submitting to Bioconductor.

Regards, Marcel