search

fmicompbio / monaLisa

binned motif enrichment analysis and visualisation
https://fmicompbio.github.io/monaLisa/
GNU General Public License v3.0
36 stars 6 forks source link

Various fixes #45

Closed csoneson closed 3 years ago

csoneson commented 3 years ago

Unrelated to the k-mer changes; fixes to address issues raised by BiocCheck

codecov[bot] commented 3 years ago

Codecov Report

Merging #45 (3b5c5fe) into master (05f403f) will increase coverage by 0.00%. The diff coverage is 99.52%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #45   +/-   ##
=======================================
  Coverage   99.51%   99.51%           
=======================================
  Files          13       13           
  Lines        2041     2071   +30     
=======================================
+ Hits         2031     2061   +30     
  Misses         10       10
Impacted Files Coverage Δ
R/motif_enrichment_wrapper.R 100.00% <ø> (ø)
R/motif_finding.R 98.33% <ø> (ø)
src/countKmerPairs.cpp 98.76% <ø> (ø)
R/pfm_comparison.R 99.18% <75.00%> (ø)
R/binning.R 100.00% <100.00%> (ø)
R/motif_enrichment_HOMER.R 99.61% <100.00%> (+<0.01%) :arrow_up:
R/motif_enrichment_kmers.R 98.93% <100.00%> (+<0.01%) :arrow_up:
R/motif_enrichment_monaLisa.R 100.00% <100.00%> (ø)
R/plotting.R 100.00% <100.00%> (ø)
R/sample_random_regions.R 100.00% <100.00%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 05f403f...3b5c5fe. Read the comment docs.

csoneson commented 3 years ago

One general thing I am not sure about: There are many occurrences of something like: attr(bins, "bin0"). Should we replace them by getZeroBin(bins)? It will probably create a bit of overhead that can be negleted, but using the interface might shield us from potential changes of the underlying implementation? On the other hand, the current code works and there are other attributes for which we do not provide any API at the moment.

Conceptually, my thought would be yes - replace by getZeroBin(bins), for the reason you give above. However, there may be more pressing issues to prioritize.