AuroreAA / ICSClust

What the Package Does in One 'Title Case' Line
GNU General Public License v3.0
0 stars 0 forks source link

Documentation comments #13

Closed VLDrenth closed 1 year ago

VLDrenth commented 1 year ago

This issue is to compile a list of comments or suggestions for the documentation.

VLDrenth commented 1 year ago

The example (given below) in select_plot() does not run without error, since the object 'res' is not defined.

X <- iris[,-5]
out <- ICS(X)

# on an ICS object
select_plot(out)
select_plot(out, type = "lines")

# on an ICS_crit object
select <- med_crit(res, nb_select = 1, select_only = FALSE)
select_plot(out, type = "lines")
select_plot(out, screeplot = FALSE, type = "lines", color = "lightblue")

I think res should be replaced by out, and the out by the result of med_crit as done below to fix the issue:

X <- iris[,-5]
out <- ICS(X)

# on an ICS object
select_plot(out)
select_plot(out, type = "lines")

# on an ICS_crit object
res <- med_crit(out, nb_select = 1, select_only = FALSE)
select_plot(res, type = "lines")
select_plot(res, screeplot = FALSE, type = "lines", color = "lightblue")
VLDrenth commented 1 year ago

In ICSClust() for the parameter nb_select the documentation states that it is required if criterion is (among others) "med_crit". However, running the following code does not produce any warning or error:

X <- iris[,-5]
ICSClust(X, nb_clusters = 2, criterion = "med_crit")
VLDrenth commented 1 year ago

In var_crit() for the return value RollVarX the description is the rolling variances which is not really clear to me. A reference is given to ICtest::ICSboot(), but the same terminology is not used there. Changing this to be consistent with ICSboot would make it easier to understand.

VLDrenth commented 1 year ago

In the function select_plot() the parameter for the width of the shading is called int. Perhaps it would be clearer to call this parameter something like 'width', especially since it does not seem to be required for this to be an integer.

VLDrenth commented 1 year ago

For the function runif_outside_range() it is hard to distill how the mult parameter in the function affects the result. Providing the actual bounds on the values that are implied by mult might be easier to read, and one could leave the explanation of the method to the details.

VLDrenth commented 1 year ago

In runif_outside_range, when using mult = 1, the method gets stuck in an infinite loop. It might be good to check in the function whether the mult value is greater than one.

VLDrenth commented 1 year ago

In scatters.R, the function ICS_scov is preceded by the following roxygen code block, which messes up the rendered documentation:

#' # reference implementation using package 'amap'
#' #' @importFrom amap W
#' #' @importFrom stats var
#' tcov_amap <- function(x, beta = 2) {
#'   # initializations
#'   x <- as.matrix(x)
#'   cn <- colnames(x)
#'   # compute inverse of maximum likelihood estimate of covariance matrix
#'   # n <- nrow(x)
#'   # S_inv <- solve(var(x) * ((n-1)/n))
#'   S_inv <- solve(var(x))
#'   V <- amap::W(x, h = 1/sqrt(beta), D = S_inv, kernel = "gaussien")
#'   # set row and column names and return scatter matrix
#'   dimnames(V) <- list(cn, cn)
#'   V
#' }

It seems parts of this should be part of the documentation of ICS_scov, but what should happen to the tcov_amap definition is not clear to me.

VLDrenth commented 1 year ago

In the documentation of ICS_lcov the return value location is explained as 'if requested, a numeric vector giving the location estimate'. However, it does not refer to when or how this can be requested.

AuroreAA commented 1 year ago

In ICSClust() for the parameter nb_select the documentation states that it is required if criterion is (among others) "med_crit". However, running the following code does not produce any warning or error:

X <- iris[,-5]
ICSClust(X, nb_clusters = 2, criterion = "med_crit")

documentation to update (it is not strictly speaking required but better to choose components)

AuroreAA commented 1 year ago

In the function select_plot() the parameter for the width of the shading is called int. Perhaps it would be clearer to call this parameter something like 'width', especially since it does not seem to be required for this to be an integer.

Agree and the arguments should be mentioned first.

AuroreAA commented 1 year ago

In var_crit() for the return value RollVarX the description is the rolling variances which is not really clear to me. A reference is given to ICtest::ICSboot(), but the same terminology is not used there. Changing this to be consistent with ICSboot would make it easier to understand.

to discuss with @klauschN

aalfons commented 1 year ago

For the function runif_outside_range() it is hard to distill how the mult parameter in the function affects the result. Providing the actual bounds on the values that are implied by mult might be easier to read, and one could leave the explanation of the method to the details.

It might be the easiest to include a figure in the help file because it's a bit cumbersome to explain in words. See here how to include a figure in a help file: https://cran.r-project.org/doc/manuals/R-exts.html#Figures

Or include examples that demonstrate this where we plot a very large sample drawn with this function with different values for mult.

aalfons commented 1 year ago

In runif_outside_range, when using mult = 1, the method gets stuck in an infinite loop. It might be good to check in the function whether the mult value is greater than one.

Indeed, we need to include a check that mult is greater than 1 and mention that in the documentation.

AuroreAA commented 1 year ago

In the documentation of ICS_lcov the return value location is explained as 'if requested, a numeric vector giving the location estimate'. However, it does not refer to when or how this can be requested.

to correct