Pass along option for full covariance matrix to Optimal_Clusters_GMM - #70
Conversation
…M_arma_AIC_BIC Co-authored-by: FMKerckhof <2431458+FMKerckhof@users.noreply.github.com>
Co-authored-by: FMKerckhof <2431458+FMKerckhof@users.noreply.github.com>
…onsistency Co-authored-by: FMKerckhof <2431458+FMKerckhof@users.noreply.github.com>
…d docs and tryCatch interface; register extra parameter for _ClusterR_GMM_arma_AIC_BIC in src/init.c; add .gitignore
…atrices Add full_covariance_matrices parameter to Optimal_Clusters_GMM
There was a problem hiding this comment.
Pull request overview
This PR adds support for full covariance matrices to the Optimal_Clusters_GMM function, enabling users to choose between diagonal and full covariance matrices when determining optimal cluster numbers via AIC/BIC criteria. The implementation correctly propagates the new parameter through all layers (R, C++, and Rcpp interfaces) and updates the AIC/BIC calculation to account for the different number of free parameters in full versus diagonal covariance matrices.
Key changes:
- Added
full_covariance_matricesparameter toOptimal_Clusters_GMMand related functions with a default value ofFALSEfor backward compatibility - Updated AIC/BIC calculations to use correct formulas for parameter counting based on covariance type
- Added comprehensive test coverage for the new functionality including backward compatibility tests
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| R/clustering_functions.R | Added full_covariance_matrices parameter to Optimal_Clusters_GMM and tryCatch_optimal_clust_GMM functions with validation and documentation |
| R/RcppExports.R | Updated GMM_arma_AIC_BIC wrapper to include the new parameter with default value |
| src/init.c | Updated function signature declaration to accept 11 parameters instead of 10, with minor formatting changes |
| src/RcppExports.cpp | Added parameter handling for full_covariance_matrices in the exported C++ function |
| src/export_inst_folder_headers.cpp | Added the new parameter to the function signature and forwarded it to the underlying implementation |
| inst/include/ClusterRHeader.h | Implemented the logic to calculate correct number of free parameters based on covariance type for AIC/BIC calculations |
| man/Optimal_Clusters_GMM.Rd | Added documentation for the new parameter and explained the impact on AIC/BIC values |
| man/tryCatch_optimal_clust_GMM.Rd | Updated function signature to include the new parameter |
| tests/testthat/test-gmm.R | Added five new test cases covering various scenarios with full covariance matrices |
| src/.gitignore | Added standard gitignore entries for compiled artifacts (*.o, *.so, *.dll) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (verbose) { Rcpp::Rcout << "iteration: " << i + 1 << " num-clusters: " << max_clusters(i) << std::endl; } | ||
|
|
||
| Rcpp::List gmm = GMM_arma(data, max_clusters(i), dist_mode, seed_mode, km_iter, em_iter, false, var_floor = 1e-10); | ||
| Rcpp::List gmm = GMM_arma(data, max_clusters(i), dist_mode, seed_mode, km_iter, em_iter, false, var_floor = 1e-10, seed, full_covariance_matrices); |
There was a problem hiding this comment.
The assignment operator is used here instead of just passing the parameter value. This should be var_floor (without the = 1e-10), as the parameter is already passed to the function and should be forwarded to GMM_arma. The current code creates an assignment expression which may not behave as intended.
| Rcpp::List gmm = GMM_arma(data, max_clusters(i), dist_mode, seed_mode, km_iter, em_iter, false, var_floor = 1e-10, seed, full_covariance_matrices); | |
| Rcpp::List gmm = GMM_arma(data, max_clusters(i), dist_mode, seed_mode, km_iter, em_iter, false, var_floor, seed, full_covariance_matrices); |
|
|
||
| In case that the \emph{max_clusters} parameter is a contiguous or non-contiguous vector then plotting is disabled. Therefore, plotting is enabled only if the \emph{max_clusters} parameter is of length 1. | ||
|
|
||
| When \emph{full_covariance_matrices} is TRUE, the AIC/BIC values will be different from when it is FALSE because full covariance matrices have more free parameters (k*(d + d*(d+1)/2)) compared to diagonal covariance matrices (k*2*d), where k is the number of clusters and d is the number of dimensions. |
There was a problem hiding this comment.
The formula for diagonal covariance matrices in the documentation is incomplete. It states "k2d" but the actual implementation uses "kd + kd + (k-1)" which equals "2kd + (k-1)". The "(k-1)" term accounts for the mixture weights and should be included in the documentation for accuracy. Similarly, the full covariance formula should also explicitly include the "(k-1)" term. The correct formulas are: diagonal = "k2d + (k-1)" and full = "k*(d + d*(d+1)/2) + (k-1)".
| When \emph{full_covariance_matrices} is TRUE, the AIC/BIC values will be different from when it is FALSE because full covariance matrices have more free parameters (k*(d + d*(d+1)/2)) compared to diagonal covariance matrices (k*2*d), where k is the number of clusters and d is the number of dimensions. | |
| When \emph{full_covariance_matrices} is TRUE, the AIC/BIC values will be different from when it is FALSE because full covariance matrices have more free parameters (k*(d + d*(d+1)/2) + (k-1)) compared to diagonal covariance matrices (k*2*d + (k-1)), where k is the number of clusters and d is the number of dimensions. The (k-1) term accounts for the mixture weights. |
| #' | ||
| #' In case that the \emph{max_clusters} parameter is a contiguous or non-contiguous vector then plotting is disabled. Therefore, plotting is enabled only if the \emph{max_clusters} parameter is of length 1. | ||
| #' | ||
| #' When \emph{full_covariance_matrices} is TRUE, the AIC/BIC values will be different from when it is FALSE because full covariance matrices have more free parameters (k*(d + d*(d+1)/2)) compared to diagonal covariance matrices (k*2*d), where k is the number of clusters and d is the number of dimensions. |
There was a problem hiding this comment.
The formula for diagonal covariance matrices in the documentation is incomplete. It states "k2d" but the actual implementation uses "kd + kd + (k-1)" which equals "2kd + (k-1)". The "(k-1)" term accounts for the mixture weights and should be included in the documentation for accuracy. Similarly, the full covariance formula should also explicitly include the "(k-1)" term. The correct formulas are: diagonal = "k2d + (k-1)" and full = "k*(d + d*(d+1)/2) + (k-1)".
| #' When \emph{full_covariance_matrices} is TRUE, the AIC/BIC values will be different from when it is FALSE because full covariance matrices have more free parameters (k*(d + d*(d+1)/2)) compared to diagonal covariance matrices (k*2*d), where k is the number of clusters and d is the number of dimensions. | |
| #' When \emph{full_covariance_matrices} is TRUE, the AIC/BIC values will be different from when it is FALSE because full covariance matrices have more free parameters (k*(d + d*(d+1)/2) + (k-1)) compared to diagonal covariance matrices (k*2*d + (k-1)), where k is the number of clusters and d is the number of dimensions. The (k-1) term accounts for the mixture weights. |
|
hi @FMKerckhof and thanks for the pull request. You are right we forgot to add the additional logical parameter Moreover, thanks for the correction of the calculation of the number of free parameters depending on covariance type in the 'GMM_arma_AIC_BIC' function Let me proceed and merge because the pull request errors are related to the github action .yml files |
Hi @mlampros - I realised after we added the full covariance matrix option to the predict function and fitting function of GMM, it was not yet propagated to the
Optimal_Clusters_GMMfunction - I used Copilot and manual revisions/testing and this PR is functional. Nevertheless, some checks fail in GH actions, but they do not seem to be directly due to my code.