diego-urgell / BinSeg

GSOC 2021. R package that performs changepoint analysis using the Binary Segmentation algorithm. Supports several statistical distributions. The model is computed in C++ and then interfaced with R via the Rcpp package.
https://summerofcode.withgoogle.com/projects/5915303348797440
MIT License
4 stars 0 forks source link

assign new column intead of using cbind #16

Closed tdhock closed 3 years ago

tdhock commented 3 years ago

hi @diego-urgell can you please change this code in the coef method

     for(j in seq_along(object@param_names)){
       jj <- 6 + (j - 1)*2
       ans <- cbind(ans, build_param(jj, jj+1, curr_segs, object@param_names[j]))
     }

to

set(ans, j=param_name, value=build_param(jj, jj+1, curr_segs, object@param_names[j])))

which should be more time/memory efficient. (your method using cbind involves re-allocating a new data table for every parameter, and this can be avoided)

tdhock commented 3 years ago

also would be good to use more informative variable names (single/double letter names like j are potentially confusing)

     for(param_index in seq_along(object@param_names)){
       param_name <- object@param_names[[param_index]]
       other_index <- 6 + (param_index - 1)*2
       set(.....)
     }
tdhock commented 3 years ago

also when there are magic numbers like "6" in the code it would be good to provide a comment explaining where it comes from. can you please explain? or even better change the code so that it is obvious and comments are no longer needed?

number_of_SOMETHING <- 6
other_index <- number_of_SOMETHING + (param_index - 1)*2
diego-urgell commented 3 years ago

Thanks @tdhock! It is much better now :)

tdhock commented 3 years ago

great