kollerma / robustlmm

This is an R-package for fitting linear mixed effects models in a robust manner. The method is based on the robustification of the scoring equations and an application of the Design Adaptive Scale approach.
28 stars 9 forks source link

"Wrong" const qualifiers in src/rlmerMatrixUtils.cpp #29

Closed jaganmn closed 1 year ago

jaganmn commented 1 year ago

Hi - I'm a co-author of R package Matrix. I've just cleaned up our C API somewhat and noticed that we wrongly used const qualifiers in arguments 1, 3, 4, and 5 of our declaration of M_cholmod_sdmult. You call M_cholmod_sdmult here,

https://github.com/kollerma/robustlmm/blob/e2b53679d46e2913d5f66fca82fec94910b6ecdc/src/rlmerMatrixUtils.cpp#L186

and here,

https://github.com/kollerma/robustlmm/blob/e2b53679d46e2913d5f66fca82fec94910b6ecdc/src/rlmerMatrixUtils.cpp#L211

where it seems to me that all of your arguments 1, 3, 4, and 5 have been declared const. A corollary is that robustlmm installation fails if we remove the const qualifiers from our prototype, as I am wanting to do.

To avoid problems with CRAN, I'll revert my prototype fix until you are able to adjust your usage in rlmerMatrixUtils.cpp. It's not urgent, so maybe just reply when it's safe to reinstate the fix ...

jaganmn commented 1 year ago

Note also that arguments 3 and 4 should be arrays of length 2, not pointers to arrays of length 1. Well, the second array element may not be referenced in real (i.e., not complex) cases, but we don't guarantee that ...

jaganmn commented 1 year ago

Incidentally, I am hoping to eventually deprecate macros MATRIX_VALID_ge_dense and MATRIX_VALID_Csparse, which you also use in rlmerMatrixUtils.cpp. You could define and use the following instead ...

#include <Rinternals.h>
int is_ge(SEXP x)
{
    static const char *valid[] = {
        "ngeMatrix", "lgeMatrix", "dgeMatrix", "" };
    return R_check_class_etc(x, valid) >= 0;
}
int is_Csparse(SEXP x)
{
    static const char *valid[] = {
        "ngCMatrix", "lgCMatrix", "dgCMatrix",
        "nsCMatrix", "lsCMatrix", "dsCMatrix",
        "ntCMatrix", "ltCMatrix", "dtCMatrix", "" };
    return R_check_class_etc(x, valid) >= 0;
}
kollerma commented 1 year ago

Hi - Thanks for the heads up and the pointers. I've created a new branch issue29 with an attempt to fix all the issues. Is there a branch of Matrix that I can check against? Or would you be able to check whether this branch solves all the problems you've mentioned? Thanks!

jaganmn commented 1 year ago

I've checked out the branch - I can confirm that it solves the problems backwards compatibly (i.e., it passes its checks independently of the changes in Matrix). It would be convenient, but not strictly necessary, to have the patched version of robustlmm submitted before we submit Matrix 1.6-2 in mid-October. I can always delay the changes in Matrix if necessary.

Thanks very much for the fast response!

kollerma commented 1 year ago

New package version is on its way to CRAN