Potential fixes for #48 (assisted by copilot) - #66
Conversation
Co-authored-by: FMKerckhof <2431458+FMKerckhof@users.noreply.github.com>
Co-authored-by: FMKerckhof <2431458+FMKerckhof@users.noreply.github.com>
Co-authored-by: FMKerckhof <2431458+FMKerckhof@users.noreply.github.com>
Co-authored-by: FMKerckhof <2431458+FMKerckhof@users.noreply.github.com>
…dictions Add support for full covariance matrix prediction in GMM
There was a problem hiding this comment.
Pull Request Overview
This PR extends the GMM prediction functionality to support full covariance matrices, addressing issue #48. The implementation adds a new C++ function to handle 3D covariance arrays while maintaining backward compatibility with diagonal covariance matrices.
Key changes:
- Added new C++ function
predict_MGausDPDF_fullfor handling full covariance matrices (3D arrays/cubes) - Enhanced
predict_GMMfunction to automatically detect and handle both diagonal and full covariance matrices - Added comprehensive test coverage for the new functionality
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| inst/include/ClusterRHeader.h | Implements core C++ logic for full covariance matrix prediction |
| src/export_inst_folder_headers.cpp | Exports the new C++ function to R |
| R/clustering_functions.R | Updates predict_GMM to handle both diagonal and full covariance matrices |
| tests/testthat/test-gmm.R | Adds comprehensive test coverage for full covariance functionality |
| src/RcppExports.cpp | Auto-generated Rcpp exports for new function |
| src/init.c | Registers new C function |
| R/RcppExports.R | Auto-generated R wrapper for new function |
| man/predict_GMM.Rd | Updates documentation to reflect new functionality |
| DESCRIPTION | Version bump for RoxygenNote |
| .Rbuildignore | Adds R project files to ignore list |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
hi @FMKerckhof and thank you for the pull request. I took a look to the following files (mainly):
The important changes are in the
We can proceed and merge your pull request but could you adjust the DESCRIPTION file and add your data as a "Github Contributor"? ( I also found a small bug in the normalization constant where the determinant is used, which I'll fix once this pull request is merged. |
…dictions add contributor info
|
@mlampros - done, feel free to merge 👍 |
|
@FMKerckhof thank you. |
Hi @mlampros - I got stuck in trying to resolve #48 so I prompted GitHub Copilot to come up with a fix for the predict function. I tested the proposed solution, and rebuilt the documentation. On the vignette example the code works smoothly, and I will validate this on a larger dataset as well.
The following code works:
I have created this as a draft PR for now, until I can validate on a larger dataset