tjh48 commented 8 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/tjh48/clusterSeq

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] 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.

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 Subversion (optionally via GitHub).

For help with submitting your package, please subscribe and post questions to the bioc-devel mailing list.

bioc-issue-bot commented 8 years ago

Hi @tjh48

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: clusterSeq
Type: Package
Title: Clustering of high-throughput sequencing data by identifying co-expression patterns
Version: 0.99.0
Depends: R (>= 3.3.0), methods, BiocParallel
Suggests: baySeq, BiocStyle
Date: 2016-01-19
Author: Thomas J. Hardcastle & Irene Papatheodorou
Maintainer: Thomas J. Hardcastle <tjh48@cam.ac.uk>
Description: Identification of clusters of co-expressed genes based on their expression across multiple (replicated) biological samples.
License: GPL-3
LazyLoad: yes
biocViews: Sequencing, DifferentialExpression, MultipleComparison

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

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

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

lshep commented 8 years ago

Thank you for your submission.

I will review and work through your code and respond back with any suggestions or comments soon. In the meantime, we would appreciate if you could work on fixing some of the WARNINGS and NOTES generated in the build report.

For the compressed data warning, if you run R CMD build --resave-data clusterSeq This will create a tarball that has the data files compressed as small as possible. Untar this file and copy the data files in it over the ones in your git working copy and then recommit those.
Please add the suggested importFrom to the NAMESPACE to fix the 'no visible global function' and updated the DESCRIPTION file as necessary importFrom("graphics", "lines", "plot") importFrom("stats", "as.dist", "cutree", "hclust", "kmeans", "median", "runif", "sd") importFrom("utils", "object.size", "read.table")
Please try and also correct the 'no visible binding for global variable' See ?globalVariables
Please consider adding the biocViews GeneExpression and Clustering if appropriate
Add non empty \value section to the man pages cD.ratThymus-data.Rd, ratThymus-data.Rd.
Add baySeq import in the NAMESPACE and remove "::" in any R code.

I will respond with any further comments or suggestions shortly. Thank you.

tjh48 commented 8 years ago

Fixed these, except for 5, in line with R manual guidance on documenting data sets:

2.1.2 Documenting data sets The structure of Rd files which document R data sets is slightly different. Sections such as \arguments and \value are not needed but the format and source of the data should be explained.

Thanks

Tom

lshep commented 8 years ago

Can you please push these changes and do a version bump in the DESCRIPTION so a new build is started.

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

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

lshep commented 8 years ago

Thank you for making those changes. Please see the following:

General:

From the build report: please update R dependency from 3.0.0 to 3.3 in Description
It seems like you are using another package you maintain baySeq, mainly the countData object. You are pulling out data a lot with the @ . You might consider if this is something not only you as a developer will be accessing but potentially the user may access, to update the baySeq::countData accessor methods to include these as we don't encourage direct access to an object internal structure. It is not a necessity but just something to keep aware.

Vignette:

There is a mismatch / extra code after the \end{document} Please remove or relocate
There are missing citations and empty footnotes
Please remove any commented out sections
The last statement seems to be an incomplete sentence
I realize that kCluster can take both a matrix and the baySeq::countData object as an argument. It would be nice if either countData object is shown as well or at least mentioned as an option. We like to show consistency and interconnectivity of bioconductor packages.

R code:

I noticed you took out summariseStability in the vignette and it is no longer exported or documented. can it be removed?
kCluster2.R seems to hold an internal function as well but it doesn't seem to be utilized anywhere. Can this be removed?

Man pages:

Please remove any "::" in the man page examples. This shouldn't be needed
Please remove any lingering comments. See example but there are others: (% Add one or more standard keywords, see file 'KEYWORDS' in the % R documentation directory)

Also, it seemed like the autobuild after the Description version bump didn't happen. This could have been on our end as we were making some updates to the build system earlier in the week but if you could check that your webhook is set up we would appreciate it.

Thank you Lori

bioc-issue-bot commented 8 years ago

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

cb103b7 Delete Read-and-delete-me 7570181 Add files via upload f2adc19 Version bump a9f045b bump

bioc-issue-bot commented 8 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: "abnormal". 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/clusterSeq_buildreport_20160922081929.html

bioc-issue-bot commented 8 years ago

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

9ede837 bump

bioc-issue-bot commented 8 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: "abnormal". 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/clusterSeq_buildreport_20160922082057.html

tjh48 commented 8 years ago

Hi Lori. Updates made.

There seems to be a problem with the builder (or my implementation of the webhook). I've pulled the current clusterSeq off github and built it on a new machine, which works perfectly well, but the bioconductor builder fails badly.

Thanks,

Tom

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

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

tjh48 commented 8 years ago

Seems to be fixed now - thanks!

lshep commented 8 years ago

Thank you for your recent updates. A few more minor changes below to address:

VIGNETTE Please add a section for installation before the library call

source("http://www.bioconductor.org/biocLite.R")
biocLite("clusterSeq")

MAN ratThymus-data.Rd Mixed " ' " and " " for matrix(mobData')

kcluster.Rd It would be nice to see a demonstration using a countData object in addition to the matrix.

R code

use seq_len()/seq_along() rather than 1:n/1:length(x) where appropriate
remove .Random.seed from clusterSeq-internal.R as it does not appear to be used anywhere.
The output created from makeClusters and makeClusterFF appear to be lists of numeric/integer. Please change the output to use the exisiting S4Vector infastructure NumericList / IntegerList from the IRanges package. This allows the output to be more structured. This can be accomplished through a simple conversion before returning the object in the functions: i.e as(current_list, "NumericList")

bioc-issue-bot commented 8 years ago

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

d52ab37 changed to use seq_len internally; IntegerLists as...

bioc-issue-bot commented 8 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, 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/clusterSeq_buildreport_20160930182342.html

bioc-issue-bot commented 8 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, 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/clusterSeq_buildreport_20161003072023.html

lshep commented 8 years ago

Hello, It looks like you saved a .Random.seed when you saved your data object in cD.ratThymus.RData . Please remove this.

> load("cD.ratThymus.RData")
> ls(all.names=TRUE)
[1] "cD.ratThymus" ".Random.seed"

This may be causing the new ERROR.

tjh48 commented 8 years ago

I don't think this has anything to do with the saved object, but appears because a baySeq object is loaded, which causes the required packages (specifically, BiocGenerics) to import into the R session. I can duplicate this behaviour thusly:

library(BiocGenerics) ls(all.names = TRUE) [1] ".Random.seed"

The errors in the build report seem to be a failure of the parallelisation on the build servers. I will look at disabling this for the vignette/examples.

lshep commented 8 years ago

Were you planning on resubmitting before the release deadline (tomorrow)?

tjh48 commented 8 years ago

Sorry - other projects had to take priority. I guess this package will have to wait for the next release.

bioc-issue-bot commented 8 years ago

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

d915074 biocParallel fixing

bioc-issue-bot commented 8 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 response

Error code 414.

Message: Request-URI Too Long.

Error code explanation: 414 = URI is too long.. ". 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/clusterSeq_buildreport_20161104101555.html

lshep commented 8 years ago

looking into this issue.

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

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

lshep commented 8 years ago

You can ignore the warning about the version number; this is on our end. I will review and get back to you. On a quick glance, it seems like with the recent changes the build and check times have almost doubled. Is this expected?

tjh48 commented 8 years ago

Not entirely expected, but probably due to changes I've made to the default BiocParallel settings for out-of-the-box running on Windows operating system.

lshep commented 8 years ago

The package has to be able to run R CMD check --no-build-vignettes in under 5 minutes. Is there a way to simplify the examples or vignette, or reduce size of the data object utilized?

lshep commented 8 years ago

Some additional points:

[ ] reducing the size of the data used in vignettes and examples would be helpful to try and reduce the time for R CMD check --no-build-vignettes
[ ] In your vignette code chunk 11 takes awhile. It also asks for 24 cores. It is recommended to use BiocParallel to detect the cores. There should be an internal check that queries the OS for max workers. If an explicit number of cores is requested, in the documentation the default is 4 cores, it should probably be reduced to <= 4. Code chunk 9 also requests 24 cores; this should also be changed to <=4. These are the calls to kCluster.
[ ] default BPPARAM should be bpparam() not SnowParam. BiocParallel detects the OS and decides which param is appropriate. Was there a reason SnowParam() was used as the default for the 'BP' arg in associatePosteriors() and kCluster(). The usual approach is to use BP = bpparam() since bpparam() will detect the OS and choose the appropriate back-end and the approprate number of cores.

bioc-issue-bot commented 7 years ago

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

1f225fc parallel fixes

tjh48 commented 7 years ago

Testing on build - will check to see time taken for examples. Build is rapid on my test machine so trying to find time on Bioconductor builds.

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

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

bioc-issue-bot commented 7 years ago

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

a2d80fc version bump

bioc-issue-bot commented 7 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/clusterSeq_buildreport_20161129062620.html

bioc-issue-bot commented 7 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/clusterSeq_buildreport_20161207084324.html

lshep commented 7 years ago

I'm not exactly sure what is causing the latex ERROR. I went on the system and built and checked the package manually and it seemed to be okay. I will accept the package but please be advised once the package is moved to the daily build system if this ERROR persists we would expect you attention in a timely manner. Please keep the issue open until you receive svn instructions via email for upload to Bioconductor. Thank you for your time and effort.

Bioconductor / Contributions

clusterSeq #105

Error response