[r] Change default insertion mode for macs peak calling, rename function

[immanuelazn]

Deprecating call_macs_peaks() to call_peaks_macs() as discussed previously. Note, is this how we should be handling deprecated APIs?

[immanuelazn]

Is this an adequate way of deprecating an interface?

[bnprks]

I've been using the lifecycle package for this. Function rename deprecation messages would use something like this:

lifecycle::deprecate_warn("0.2.0", "draw_trackplot_grid()", "trackplot_combine()")

Another nice detail could be keeping a bit more docs on the old function's docstring. This helps for if people run ?call_macs_peaks or if they access an old URL for the function docs. e.g.

#' Call peaks using MACS2/3
#'
#' @description
#' `r lifecycle::badge("deprecated")` 
#'
#' This function has been renamed to `call_peaks_macs()`
#' @export
#' @keywords internal

This way it won't be indexed in the list of functions, but it will be possible to access the function docs directly

[bnprks]

Overall looks good, just a couple small comments below. I'm assuming that we'll save working on the Windows C++ threading for another PR?

[bnprks]

I've been using the lifecycle package for this. Function rename deprecation messages would use something like this:

lifecycle::deprecate_warn("0.2.0", "draw_trackplot_grid()", "trackplot_combine()")

Another nice detail could be keeping a bit more docs on the old function's docstring. This helps for if people run ?call_macs_peaks or if they access an old URL for the function docs. e.g.

#' Call peaks using MACS2/3
#'
#' @description
#' `r lifecycle::badge("deprecated")` 
#'
#' This function has been renamed to `call_peaks_macs()`
#' @export
#' @keywords internal

This way it won't be indexed in the list of functions, but it will be possible to access the function docs directly

[immanuelazn]

Cool, I'll make the changes with the better way of styling!

I think it would make sense to do the multithreading as a separate PR as that requires work more on the internal logic, rather than interface/documentation

[immanuelazn]

Styling changes from deprecation finished!

[bnprks]

Overall, this looks quite good and is ready to merge.

Something we should discuss separately: do we knew why we missed the default argument issue initially despite comparing outputs against ArchR. Do you think our checks didn't cover this or weren't running with insertion_mode set to the default?

[immanuelazn]

Yup, so the results were compared in this file . I passed in the argument for both in insertion_mode within this test case but didn't change the default param on the macs, and bedfile themselves.