Stab sel

[machlabd]

added stability selection functions, made findmotifHits accept Granges object, and updated the vignette accordingly.

[mbstadler]

Wow, great work!

I am going through each file and I am writing my feedback as comments here. I may also correct typos, e.g. in the documentation and push them, but I will not modify the code itself.

compbio-lisa.Rproj:

• It's better not to track the .Rproj file on github. It contains user settings and this may lead to git pull to overwrite mine with yours, for example.

R/stability_selection.R:

• lines 39, 40: this check should not be needed (the package depends on glmnet)
• lines 149, 151 and 152: This is better checked using base::inherits()
• line 153: why is it commented out? It seems a reasonable test to me.
• line 157: should it not take the names from PWMs instead? Otherwise you loose here any TFs with zero hits, which you then have to bring back below (line 168 and following)
• lines 158,159: Would l <- table(TFs, seqs) also work (but be faster)? This may need a rewrite of lines 167-174, though.
• line 167: you use parallel::mclapply without an @importFrom parallel mclapply
• line 177: I am not sure I would remove those here. Users may expect to get a matrix with the number of rows corresponding to the number of peaks. Probably, they have to be filtered out in a fitting function instead.

DESCRIPTION: looks good - I just modified the Description to better represent the current content of lisa, and I added parallel to Imports (see comment above).

[mbstadler]

NAMESPACE and .Rd files: I am not commenting on these files as they are autogenerated by Roxygen.

R/plotting.R:

• lines 287-298: Maybe you could add a link / @seealso to the function that creates the stabs_object
• line 299: What about renaming plot_stabilityPaths to plotStabilityPaths, to be more consistent with the plotBinScatter, plotBinHist, plotBinDensity etc.
• line 302: better use base::inhertis()
• lines 305-309: It seems this will unconditionally overwrite the cols argument
• line 312: You use matplot without @importFrom graphics matplot
• line 330: The return value is actually not the barplot, but the return value of legend() (the last function you call). Maybe there is a more useful return value? If not, you might as well return invisible(NULL) or invisible(TRUE) just to signal success
• line 332: What about renaming barplot_selectionProbability to plotSelectionProb or similar for consistency?
• line 346: You use isEmpty without importing from S4Vectors. Here, I think you should not use isEmpty at all, as it makes lisa depend on S4Vectors just for this small function. Maybe you could use if (length(TF_prob) == 0) (for a vector), if (any(dim(x) == 0L)) (for a matrix) or similar

[mbstadler]

tests/testthat/test_randomized_stabsel.R:

• line 20: It's good to test that the signal is selected, but what about testing if the noise is not selected?

vignettes/lisa.Rmd:

• general question (also maybe to discuss with @LukasBurger ): Is it better to have a single vignette, or multiple vignettes each focused on one task (e.g. predict motif hits, calculate binned motif enrichment, perform stability selection, ...)?
• lines 241-268: This is legacy code/comments and has been implemented (I should probably remove it)
• lines 290, 291: I think you don't need mustWork here (you know the files that are in your own package)
• line 299 and following: Is it simpler to use just table() (as in the example on line 237) than using a specific function (get_numberOfTFBS_perSeqName)? What else does the function do. If it is not needed it may be simpler for the user to not have to learn about it.
• lines 331, 332: you use Biostrings::oligonucleotideFrequence without explicit library(Biostrings). I would add this here for clarity, and I already added Biostrings to Imports in DESCRIPTION
• lines 358, 363: in case you renamed the plotting functions (see above), they will also have to be fixed here
• lines 368: I would also include an explicit library(ComplexHeatmap) here

[mbstadler]

R/motif_finding.R:

• lines 405, 408: these should be check with base::inherits()

example data:

• this is now nice and small - great!

R CMD check:

• I had to add S4Vectors to Imports (was my fault) - that also means you can ignore my comment from above about not using S4Vectors::isEmpty to avoid the dependency
• I removed duplicate package names in multiple fields within DESCRIPTION (were probably put there by me...)
• check now warns (some of this is already mentioned above):

  barplot_selectionProbability: no visible binding for global variable
   'stabs'
  barplot_selectionProbability: no visible global function definition for
   'isEmpty'
  get_numberOfTFBS_perSeqName: no visible global function definition for
   'mclapply'
  glmnet.randomized_lasso: no visible global function definition for
   'model.matrix'
  glmnet.randomized_lasso: no visible global function definition for
   'runif'
  glmnet.randomized_lasso: no visible global function definition for
   'predict'
  Undefined global functions or variables:
   isEmpty mclapply model.matrix predict runif stabs
  Consider adding
   importFrom("stats", "model.matrix", "predict", "runif")
  to your NAMESPACE file.

• test currently fails on xenon6 with (only needs to be fixed if this function stays):

   OK: 27 SKIPPED: 0 FAILED: 1
   1. Error: get_numberOfTFBS_perSeqName() works properly (@test_get_numberOfTFBS_perSeqName.R#4) 

• check warns about:

  Undocumented arguments in documentation object 'plot_stabilityPaths'
  'lwd' 'lty' 'ylim'

These seem easy to fix - let me know if you would like me to also work on some of them.

[mbstadler]

I just fixed the unit test for get_numberOfTFBS_perSeqName

[machlabd]

Thank you for the feedback! So far I've incorporated fixes into R/plotting.R:

• added @seealso stabsel to plot function
• renamed to plotStabilityPaths and plotSelectionProb for consistency
• used base::inherits for checking stabsel class
• fixed cols overrite
• ? @importFrom matplot is already there
• added invisible(TRUE) to ends of plotting functions
• ? isEmpty comment ignored for now.

[machlabd]

One other comment I forgot: In the stability selection procedure I now use homer 4.10, but the rest of the vignette uses 4.8. Should we be consistent with one of the two?

[machlabd]

R/stability_selection.R:

• removed loading of glmnet check in glmnet.randomized_lasso
• added base::inherits for checkcing classes of objects
• no need for @importFrom parallel mclapply since the function that uses it has been deleted. --> also removed from imports in decription file.
• commented out get_numberOfTFBS_perSeqName function, and will use table() instead in the the vignette.

tests/testthat/test_randomized_stabsel.R:

• added all(!(seq(1,ncol(X),1)[-s_cols] %in% ss$selected)) for expecting noisy variables not to be selected

R/motif_finding.R:

• added base::inherits to check for classes.

vignettes/lisa.Rmd:

• added table() function when getting predictor matrix.
• removed mustWork=TRUE
• added library(Biostrings) and library(ComplexHeatmap)
• ? should Biostrings e moved from imports to suggests in decsription

[mbstadler]

I have implemented some of your suggestions in the vignette, and also removed some warnings about missing imports by adding them in R/stability_selection.R (Roxygen will add them to NAMESPACE).

Biostrings needs to be in Imports rather than Suggests, as it is used e.g. by findMotifHits.

I will now look into unit test coverage.