search

stuart-lab / signac

R toolkit for the analysis of single-cell chromatin data
https://stuartlab.org/signac/
Other
330 stars 88 forks source link

Suggestion on argument name "max.downsample" in CoveragePlot and related functions #1791

Open G3N0M3 opened 1 month ago

G3N0M3 commented 1 month ago

Nothing really major, but I noticed that the argument max.downsample in the function CoveragePlot (and its relatives) might be slightly misleading in terms of its role.

In parts of the code where max.downsample and downsample.rate are compared, the higher value between the two is selected:

window.size = width(x = region)
sampling <- ceiling(x = max(max.downsample, window.size * downsample.rate))
coverages <- slice_sample(.data = all.data, n = sampling)

If max.downsample is higher, the resulting plot shows at least that many data points. This seems more like a minimum threshold rather than a maximum, which the current name might suggest.

The documentation in visualization.R script also notes: @param max.downsample Minimum number of positions kept when downsampling. Although it's explained in the documentation, I believe a name like min.downsample, or something that reflects its role as a minimal threshold would make functions more intuitive.

As an end user, I'm not fully aware of the difficulties involved in making this change, but I wanted to raise it as a suggestion.

Thanks for the wonderful package! Hope this feedback was helpful.