CrossICC - Githubissues

bioinformatist commented 5 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/bioinformatist/CrossICC

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

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

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: "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 build report for more details.

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:

9bc9aed fix bug by no.same b17ccc0 v0.99.19

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:

0d7cdab v0.99.20

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.

Liubuntu commented 5 years ago

Hi @bioinformatist ,

I'll do the initial review and hopefully have it out in 2 weeks. In the mean time, please read the building report carefully, and try to fix the warning / notes as much as possible. Otherwise, they will still be included in the initial review.

Best, Qian

bioinformatist commented 5 years ago

@Liubuntu Dear Qian, There's only one warning with few notes now. I'll try my best to fix notes but I think the warning should be kept, for the latest version of Microsoft R Open is 3.5.3, if I set the R version up to 3.6 or higher in DESCRIPTION, the users using MRO cannot use our package. I'll keep an eye on MRO's version, and once it release version 3.6, I'll fix this warning ASAP.

Thank you!

Best, Sun

bioinformatist commented 5 years ago

@Liubuntu Hi Qian, any news?

Liubuntu commented 5 years ago

Hi @bioinformatist ,

I'm working on reviewing it already. Will send out the reviews shortly.

Cheers, Qian

Liubuntu commented 5 years ago

Hi @bioinformatist ,

Please see below for the initial review. Please seek to fix all / most of the issues and comment back here for any questions / comments / explanations. Bump version in DESCRIPTION file with (group of) commits and updates so that the building machine catches the changes and triggers a new build.

Best, Qian

Liubuntu commented 5 years ago

DESCRIPTION

Title: Should it be "Iterative" (or possibly "Interactive")?
Description: Should be a relatively short but detailed overview of what the package functionality entails. Would be good to use 1-3 sentences. Now it replicates the "Introduction" of vignette.
License: If you are using a standard license (e.g., GPL-3), specify it in License: GPL-3 field would suffice. If having additional / non-standard license, use License: GPL-3 | file LICENSE or your current one.
LazyData: For packages that include data, we recommend not including LazyData: TRUE. This rarely proves to be a good thing. In our experience it only slows down the loading of packages with large data.
biocViews: Check carefully if all these terms are relevant to your package.

NAMESPACE

Looks good!

Liubuntu commented 5 years ago

R/

IMPORTANT:

Many of the utility functions were not actually called or used in your exported functions. Please go through your R scripts and remove them if necessary and keep the R folder clean. (e.g., the .plot.mergeCoeff, .summary.mergeExpressionSet, get.matrices, centroid.pairwise, cluster.centroid etc.)
Support SummarizedExperiment class as rectangular data, with rows being features, and columns being samples.

"CrossICCInput()"

L345: remove == TRUE to avoid double checking.
L354: Typo in printed message "a charachter".

"CrossICC()"

L32: it's not recommended to use the current directory as default for generating output files. Use tempdir() instead, and allow flexibility for users to define their own output directory. Update Rscript, documentation and vignette accordingly.
L75-76: It's not recommended to redirect the working directory inside the R script. Use file.path() to write results into the provided output.dir.
L120: under the if() conditions, you can get a new study.names. So you only need the names(platforms) assignment once by removing the L120:121 (Finish the if condition by }, remove the else section, and do the assignment).
L135: Seems that the max.K argument takes the value of # of samples (when <10) by default. Consider document this default behavior or print a message in the scripts when this happens.
L205:211, avoid using 1:k, use seq_len or seq_along for robustness.
L281: It's suggested to add an argument like overwrite=TRUE and let it be users decision.
L291: the error message didn't give any useful info about uninstalled but required packages for using shiny. Seems that the lapply would stop when it reaches the first uninstalled but suggested package. It would be suggested to return a list so users could install all at once, instead of testing one by one.
L298: Again, it's not encouraged to use setwd() inside the R script. Print the necessary message showing the path of the generated results.

"runFAIME()"

SummarizedExperiment is a newer / update version of ExpressionSet. For better interoperability, it's highly recommended to support this input format, so your package could easily expand usage on RNA-seq or other kind of sequencing data, and interoperates / communicates better with more recent Bioconductor packages.
L68,72, avoid using 1:k, use seq_len or seq_along instead for robustness.
Seems this is just an utility function used internally, it's recommended to prefix unexported function with ., e.g., .runFAIME, similar to FAIME() function here.

"ssGSEA"

x argument should support SummarizedExperiment as stated above. Update the documentation accordingly.

"predictor()"

pre.dat should accommendate SummarizedExperiment as stated above.
L24: typo in "continute".

"check.length()"

L262:290 Highly duplidate code. Try simplify:

outcome1 <- event1 <- NULL
if (method == "linear") {
outcome1 <- out[[i]]
} else if (method == "logistic") {
event1 <- out[[i]]
} else if (method == "cox") {
outcome1 <- out[[i]]
event1 <- out2[[i]]
}
result1 <- apply(exprs1, 1, .mergemodel, outcome = outcome1, event = event1, method = method)
for (gene in seq_len(nuid)) {
beta[gene, i] <- result1[[gene]][[1]]
stdbeta[gene, i] <- result1[[gene]][[2]]
zscore[gene, i] <- result1[[gene]][[3]]
}

".plot.mergeCoeff()"

SUGGESTION: consider using switch() here for returning different values with certain conditions.

GENERAL:

prefix utility (/unexported) functions with .. (functions in "utils.R").
avoid using 1:k, use seq_len or seq_along instead for robustness. Check through your package, besides those I mentioned above.

Liubuntu commented 5 years ago

man/

CrossICCInput:

Remove "Title" from the documentation title and description.
For the "Value" section, use "matrices" instead of "matrixs" in plural format.

CrossICC package documentation:

Remove / replace the "foo", "bar", "baz" in the documentation for corresponding functions.

CrossICC:

Add default values (if any) in the parameter documentation. Also specify the input data type for parameters. (numeric, / character, etc).

summaryCrossICC:

Remove "Title" from the documentation title and description.

predictor:

Seems that you didn't define any S4 classes here for the returned value of CrossICC() function, so it's not appropriate to say "a CrossICC object" here. Instead, model is a list that was generated by the CrossICC() function, containing results of xxx.

centroidOfcentroid:

Remove "Title" from the documentation title and description.

GENERAL:

Remove "Title" from the documentation title and description. Go through your documentations besides those I have mentioned above.

Liubuntu commented 5 years ago

vignette

CrossICC.Rmd:L38. Remove the "(coming soon)". Write as if it was already included into Bioconductor.

unit tests

SUGGESTION: It's recommended to add more contents in the unit tests for a more robust R package.

bioinformatist commented 5 years ago

@Liubuntu Thanks a lot, Qian! I'll fix them as possible as I can. Once finished, I'll notice you here.

Liubuntu commented 5 years ago

Hi @bioinformatist ,

Just a reminder that we would usually expect responses, e.g., package updates / comments / question, within 2 weeks of official review. Please kindly let us know the package status so that we can keep this issue open.

Please focus on the important issues and make appropriate commits, and version bumps, so that the building machine could catch those and trigger a new build.

Best, Qian

bioinformatist commented 5 years ago

@Liubuntu Sure, sorry for recently I have been working hard with another project and it came to a key stage. I'll update CrossICC soon and please keep this issue open, thanks!~~~

Best, Sun

bioc-issue-bot commented 5 years ago

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

878ebf0 remove useless functions 7962065 Update license and description bfd710a remove useless package man; fix man page; add over... 7b4b766 fix doc and spelling error de3b62a use seq_len instead 4c66c5b changed as already in Bioconductor f72f838 change docs c1c04c8 v0.99.21

bioinformatist commented 5 years ago

@Liubuntu Dear Qian,

Below are my responses:

For those in DESCRIPTION

Sorry for there's a spelling error in title. Fixed.
Description: A shorter version was provided.
License field as well as file were changed.
LazyData: TRUE was removed.
All terms in biocViews were relevant to our package.

For those in `R/`

Some useless functions were removed.
SummarizedExperiment class need more type check during data pre-processing. May I import it later?
We provided a parameter output.dir for function CrossICC(), which DO allow arbitrary user-defined output directory. And I believe that tempdir() is not a good design for our package, for a shiny app was provided by us, which need load data from a fixed .rds R object file, and the shiny should allow to be called anytime but free from run the main function again.

Others were fixed.

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:

38d7859 recover from removing lazy load 342bbac v0.99.22

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.

bioinformatist commented 5 years ago

@Liubuntu Still only one warning caused by R version. Please provide us further advise. Thank you in advance~:smile:

Liubuntu commented 5 years ago

Hi @bioinformatist ,

Please see the following for some additional issues. Some are carried from the initial review.

Best, Qian

R/CrossICC.R

the argument of output.dir should be passed into the script, instead of using ~ directly. Otherwise, the user defined directory will be disabled.
I'm not an expert of using shiny in the R package. But if the shiny app load data only from a fixed .rds file, how are the users supposed to pass their own results if in another directory? Seems the shiny app was only called insided the CrossICC() function, it not hard to record the output.dir, and get the final file path as file.path(output.dir, CrossICC.object.rds). Possible to pass this into the shiny app? (L295)

`inst/shiny/server.R`

L69 is reading a fixed file folder. Same question as above.

R/ssGSEA.R R/runFAIME.R

The support of a SummarizedExperiment should be easy, by just adding the following check in runFAIME() function:
```
if (is(dat, "SummarizedExperiment"))
dat <- assay(dat)
```
Same applies to the predictor.R.
runFAIME.R L60, correct the zz in the error message.
SUGGESTION: Since the runFAIME function is not exported, and the arguments are mainly passed from ssGSEA, so it's not suggested to have the argument checks (for format or values) inside the non-exported function runFAIME but rather in the exported function ssGSEA. Also the argument names are not consistent between these 2 functions, and the error message may not directly point users to the correct argument.

R/MergeMaid.R (unaddressed from first review)

is .model.outcome() function used anywhere in the package, if no, remove it. If yes, reply here, and simply the highly duplicated code in L260:L287.

outcome1 <- event1 <- NULL
if (method == "linear") {
    outcome1 <- out[[i]]
} else if (method == "logistic") {
    event1 <- out[[i]]
} else if (method == "cox") {
    outcome1 <- out[[i]]
    event1 <- out2[[i]]
}
result1 <- apply(exprs1, 1, .mergemodel, outcome = outcome1, event = event1, method = method)
for (gene in seq_len(nuid)) {
    beta[gene, i] <- result1[[gene]][[1]]
    stdbeta[gene, i] <- result1[[gene]][[2]]
    zscore[gene, i] <- result1[[gene]][[3]]
}

Is .plot.mergeCoeff() function used anywhere? specify here or remove.
Is .fdr.mergeCor function used anywhere? specify here or remove.
Is .plot.mergeExpressionSet function used anywhere? specify here or remove.
Is .hist.mergeCor function used anywhere? specify here or remove.

bioc-issue-bot commented 5 years ago

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

d54f4fc shiny can ONLY use fix dir d177b56 v0.99.23

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.

bioinformatist commented 5 years ago

Hi @Liubuntu , All were fixed while since shiny can only get rds file at a fixed dir (cannot open a new session with current CrossICC result), users must use default output.dir as the fixed position when they plan to call shiny. I have note this in man page. Any further suggestions? Thank you!

Best, Sun

Liubuntu commented 5 years ago

Hi @bioinformatist ,

Please address some easy correctable "NOTE"s in the building report to make the package more robust. e.g.,

Avoid sapply(); use vapply()
Avoid 1:...; use seq_len() or seq_along()

It's not totally incorrect for using current working directory as the default folder for output files. The problem is that we would like to avoid the home directory contamination by generating any arbitrary files whenever the "R CMD build/check" is called. For examples, Bioconductor daily build of packages calls "R CMD build/check" every day for all packages, the files generated from package examples / vignette will soon accumulate to consume the working space, and requires manual removal to clean up. To avoid the home directory contamination, you have 2 options:

Option 1 (easy fix): keep the current default value unchanged, in CrossICC(out.dir = "~"), but set the output.dir as temporary directory in all your examples and vignette where the CrossICC(outdir = tempdir()) was called, so that your example will only generate files in the temporary dir and don't contaminate working directory (e.g., for Bioconductor building machine).

Option 2 (improves the robustness and flexibility, recommended!):

you are using out.dor = "~" as default mainly because the shiny::runApp function doesn't support passing of arbitrary arguments (e.g., the user-defined directory for CrossICC results) into the shiny app, there are some workaround that you can actually fix it:
1. Set the output.dir as temporary directory.
2. write the shiny::runApp as a stand alone function, using global variable to pass the output directory into the runApp.
Since the use.shiny can only be used inside the crossICC function. It might be good to separate it out as a stand-alone function, so that the users doesn't need to calculate the result again before they decide to use the shiny app for simply checking results. In this way, define a function like this: https://github.com/rstudio/shiny/issues/440#issuecomment-40646269, and use the .GlobalEnv to pass the output directory info before calling runApp. e.g.,
```
runShiny <- function(dir, ...) {
  .GlobalEnv$.CrossICCdir <- dir
  on.exit(rm(.CrossICCdir, envir = .GlobalEnv))
  shiny::runApp(system.file("shiny", package = "CrossICC"))
}
```
You can also use the shinyOptions(), which is similar to the global environment variable. Check ?shiny:shinyOptions for more details.

bioc-issue-bot commented 5 years ago

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

dd8bb9e add newline for too long example b2e0c2f rewrite code for robustness 99267b7 v0.99.24

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:

ee65958 use tempdir() instead of root of home 0fe3fdc v0.99.25

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:

4ec20a3 use tempdir() instead of home dir 81e3216 v0.99.26

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.

bioinformatist commented 5 years ago

Hi @Liubuntu , All were satisfied, with only few notes now. Thank you for very detailed suggestions!

Best, Sun

Liubuntu commented 5 years ago

@mtmorgan , the unaddressed warning was explained here: https://github.com/Bioconductor/Contributions/issues/1159#issuecomment-516495444

mtmorgan commented 4 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/bioinformatist.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("CrossICC"). The package 'landing page' will be created at

https://bioconductor.org/packages/CrossICC

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

CrossICC #1159

DESCRIPTION

NAMESPACE

R/

man/

vignette

unit tests

For those in DESCRIPTION

For those in `R/`

R/CrossICC.R

`inst/shiny/server.R`

R/ssGSEA.R R/runFAIME.R

R/MergeMaid.R (unaddressed from first review)

Bioconductor / Contributions

CrossICC #1159

DESCRIPTION

NAMESPACE

R/

man/

vignette

unit tests

For those in DESCRIPTION

For those in R/

R/CrossICC.R

inst/shiny/server.R

R/ssGSEA.R R/runFAIME.R

R/MergeMaid.R (unaddressed from first review)

For those in `R/`

`inst/shiny/server.R`