Header clean-up to fix lme4/lme4#745

[jaganmn]

I would summarize the changes as follows:

• Make CHOLMOD support optional by excluding the corresponding headers from RcppEigen.h.
• Refactor RcppEigenCholmod.h and RcppEigenStubs.cpp (renamed from *.h) to use Matrix API, not relying on static copies. Packages must include RcppEigenCholmod.h in addition to RcppEigen.h to enable CHOLMOD support. They must compile RcppEigenStubs.cpp if including RcppEigenCholmod.h.
• Remove strong dependency on Matrix from DESCRIPTION and NAMESPACE. Packages including RcppEigenCholmod.h must have LinkingTo: RcppEigen, Matrix.

Note that it was necessary to modify two files under inst/include/Eigen directly, mainly because the Matrix API prefixes its CHOLMOD stubs.

[jaganmn]

Thanks - I expect lme4 to break, but I'll be curious about which others ...

[jaganmn]

Hmm ... AFAICT, all of this code can be removed, too:

https://github.com/jaganmn/RcppEigen/blob/rcppeigen_cholmod/inst/include/RcppEigenCholmod.h#L35-L82

It seems to have been wrongly copied from lme4:

https://github.com/jaganmn/lme4/blob/rcppeigen_cholmod/src/lme4CholmodDecomposition.h#L75-L120

I say "wrongly" because the factor() method used here:

https://github.com/jaganmn/RcppEigen/blob/284934415b3d7002d6c2fa677ef0b646488fc269/inst/include/RcppEigenCholmod.h#L41

is provided by lme4 and not by Eigen. Compare:

https://github.com/jaganmn/lme4/blob/rcppeigen_cholmod/src/lme4CholmodDecomposition.h#L20-L73

https://github.com/jaganmn/RcppEigen/blob/284934415b3d7002d6c2fa677ef0b646488fc269/inst/include/Eigen/src/CholmodSupport/CholmodSupport.h#L584-L635

Speaking as a non-expert, I am a bit surprised that the compiler doesn't complain about usage of non-existent methods ...

Edit: I have updated the PR, removing that code. The rev. dep. check should not be affected, because any package depending on the removed code (my guess is that none do) will already be broken due to removal of CHOLMOD support.

[eddelbuettel]

Reverse dependency check went fairly well. Six failures, of which four were 'temporary' as the package needed build-dependencies and passed once these were added. The two remaining once were lme4 (which you expected) and OpenMx. That is a big and complex package -- could you possibly look into what ails it now and whether we can aid it with a patch?

The reverse dependencies started to run before you added your third commit here. I have now updated the package and installed that updated package. lme4 still fails to install, as does OpenMx.

[jaganmn]

Thanks, Dirk. The patched version of lme4 should pass its checks. I'll take a look at OpenMx, hopefully today.

[eddelbuettel]

One question though: could or should the 'stub' be a header fiile (likely with inline statements) to make sure along with including RcppEigenCholmod.h. ?

[jaganmn]

One question though: could or should the 'stub' be a header fiile (likely with inline statements) to make sure along with including RcppEigenCholmod.h.?

Matrix/inst/include/Matrix_stubs.c does not provide an option for inlining. For Matrix 1.6-2, I could add something like

#ifdef R_MATRIX_ENABLE_INLINE
# define R_MATRIX_INLINE inline
#else
# define R_MATRIX_INLINE
#endif

and then add R_MATRIX_INLINE to each definition. Then packages wanting inlined versions could define R_MATRIX_ENABLE_INLINE before including RcppEigenStubs.(h|cpp). But they would need LinkingTo: Matrix (>= 1.6-2), and we might have to co-ordinate the Matrix release, too.

I guess the file extension is ultimately up to you. If you decide on *.h then I'll need to update the PRs filed with lme4 and OpenMx. *.h would make sense if the stubs were inlined unconditionally ...

[eddelbuettel]

Ok -- I was mostly coming from the 'want to make it as easy as possible' angle and copying a .c file seems like it is ever so slightly more work. But still no biggie. So no worries.

[bbolker]

Bump. I've reverted the change to lme4 for now, but it would be great to push this version to CRAN (if the OpenMX maintainers are ready too) so we can move ahead and it doesn't fall through the cracks ...

[bbolker]

Thanks. My only concern is that I haven't done reverse-dependency checks (takes 15-20 minutes to dust off my pipeline for doing it, a few hours of computation, and an hour or so to sort through false positives etc.), but I think I should be able to get away with stating that I don't think they should be necessary because there is no expectation that any changes would change behaviour of existing code (we have added a feature, changed documentation, made an error message more informative ...)

[eddelbuettel]

Passed pre-test at CRAN and got the email that automated reverse dependency checks have been launched.

[jaganmn]

@bbolker If you've not already submitted a tarball, then you can consider (as I think Dirk has done) a minimal diff update applying only the PR and excluding other changes on your trunk since last release.

[eddelbuettel]

@bbolker I presume you followed up to Uwe's email and sent an lme4 tarball for CRAN to test with?

[bbolker]

Yes, just now. (I've been busy for the last 6 hours or so.) At Uwe's request I submitted the tarball through the usual submission page.

[eddelbuettel]

Ok, thanks to Uwe the new release of RcppEigen is now on, it needed no more changes and I pushed the pending changes from that upload.

@jaganmn I riffed together a ChangeLog and NEWS entry for your PR, and I forgot you had of course left a description here in this PR and its initial paragraph. If you think what we have now needs refining drop me a line or do a PR. May not get to CRAN all that quickly but if a changes is needed we may as well put it into the repo. Big thanks once again for your careful PR.

[jaganmn]

I'm not sure. If you intend to write a blog post then maybe the details can go there. Otherwise yes I can create another PR. Perhaps more important than the ChangeLog is to document things in the header itself.

[eddelbuettel]

Sure, sure, just wanted to make sure you were aware of changes I made in your stead given that your PR lacked a ChangeLog (I am soooo old school that I still use these and of course "kids these days" have no idea what I refer to).

So consider this mostly a heads-up. But more documentation is good. Maybe even a quick Rcpp Gallery about how to do 'modern CHOLMOD use' ?

[jaganmn]

Yep - I saw the commit and was glad because I'd forgotten. My intention was to add such a commit after you approved the other changes but time slipped away. I'll add some documentation when I get a chance - which might be 2-3 weeks from now, ideally before I forget the details of what I am documenting.

[eddelbuettel]

All good, and my bad for not nagging you. I usually remember to do that but this PR dragged on a little longer than usual. Again, my fault, not yours.

[andrjohns]

I've been re-running the reverse-dependency checks for the Eigen 3.4.0 update, and these changes are causing failures with lme4 (full build log below). This is using the #102 PR with the changes to master merged in, which you can access directly on my fork.

Is this something that would need fixing in RcppEigen or lme4?

> install.packages("lme4", type="source")
trying URL 'https://cran.rstudio.com/src/contrib/lme4_1.1-35.1.tar.gz'
Content type 'application/x-gzip' length 2942848 bytes (2.8 MB)
==================================================
downloaded 2.8 MB

* installing *source* package ‘lme4’ ...
** package ‘lme4’ successfully unpacked and MD5 sums checked
** using staged installation
** libs
using C++ compiler: ‘Apple clang version 15.0.0 (clang-1500.1.0.2.5)’
using SDK: ‘MacOSX14.2.sdk’
clang++ -arch arm64 -std=gnu++17 -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG  -I'/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/Rcpp/include' -I'/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/RcppEigen/include' -I'/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/Matrix/include' -I/opt/R/arm64/include   -DNDEBUG -DEIGEN_DONT_VECTORIZE -fPIC  -falign-functions=64 -Wall -g -O2  -Wno-unknown-warning-option -Wno-enum-compare -Wno-ignored-attributes -Wno-unused-local-typedef -Wno-sign-compare -Wno-unused-function -Wno-unneeded-internal-declaration -Wno-unused-but-set-variable -Wno-unused-variable -Wno-infinite-recursion -Wno-unknown-pragmas -Wno-unused-lambda-capture -Wno-deprecated-declarations -Wno-deprecated-builtins -Wno-unused-but-set-variables -Wno-unused-lambda-capture -ftemplate-backtrace-limit=0 -Wno-nonnull -Wno-unused-but-set-variable -c external.cpp -o external.o
In file included from external.cpp:8:
In file included from ./predModule.h:13:
In file included from ./lme4CholmodDecomposition.h:13:
In file included from /Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/RcppEigen/include/RcppEigenCholmod.h:33:
In file included from /Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/RcppEigen/include/Eigen/CholmodSupport:39:
/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/RcppEigen/include/Eigen/src/CholmodSupport/CholmodSupport.h:179:1: error: use of undeclared identifier 'cholmod_start'; did you mean 'M_cholmod_start'?
EIGEN_CHOLMOD_SPECIALIZE0(int, start)
^
/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/RcppEigen/include/Eigen/src/CholmodSupport/CholmodSupport.h:174:101: note: expanded from macro 'EIGEN_CHOLMOD_SPECIALIZE0'
    template<typename _StorageIndex> inline ret cm_ ## name       (cholmod_common &Common) { return cholmod_ ## name   (&Common); }
                                                                                                    ^
<scratch space>:25:1: note: expanded from here
cholmod_start
^
/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/Matrix/include/Matrix/cholmod.h:1133:24: note: 'M_cholmod_start' declared here
R_MATRIX_INLINE    int R_MATRIX_CHOLMOD(start)(
                       ^
/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/Matrix/include/Matrix/cholmod.h:1057:34: note: expanded from macro 'R_MATRIX_CHOLMOD'
#define R_MATRIX_CHOLMOD(_NAME_) M_cholmod_ ## _NAME_
                                 ^
<scratch space>:16:1: note: expanded from here
M_cholmod_start
^
In file included from external.cpp:8:
In file included from ./predModule.h:13:
In file included from ./lme4CholmodDecomposition.h:13:
In file included from /Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/RcppEigen/include/RcppEigenCholmod.h:33:
In file included from /Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/RcppEigen/include/Eigen/CholmodSupport:39:
/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/RcppEigen/include/Eigen/src/CholmodSupport/CholmodSupport.h:180:1: error: use of undeclared identifier 'cholmod_finish'; did you mean 'M_cholmod_finish'?
EIGEN_CHOLMOD_SPECIALIZE0(int, finish)
^
/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/RcppEigen/include/Eigen/src/CholmodSupport/CholmodSupport.h:174:101: note: expanded from macro 'EIGEN_CHOLMOD_SPECIALIZE0'
    template<typename _StorageIndex> inline ret cm_ ## name       (cholmod_common &Common) { return cholmod_ ## name   (&Common); }
                                                                                                    ^
<scratch space>:27:1: note: expanded from here
cholmod_finish
^
/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/Matrix/include/Matrix/cholmod.h:1101:24: note: 'M_cholmod_finish' declared here
R_MATRIX_INLINE    int R_MATRIX_CHOLMOD(finish)(
                       ^
/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/Matrix/include/Matrix/cholmod.h:1057:34: note: expanded from macro 'R_MATRIX_CHOLMOD'
#define R_MATRIX_CHOLMOD(_NAME_) M_cholmod_ ## _NAME_
                                 ^
<scratch space>:328:1: note: expanded from here
M_cholmod_finish
^
In file included from external.cpp:8:
In file included from ./predModule.h:13:
In file included from ./lme4CholmodDecomposition.h:13:
In file included from /Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/RcppEigen/include/RcppEigenCholmod.h:33:
In file included from /Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/RcppEigen/include/Eigen/CholmodSupport:39:
/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/RcppEigen/include/Eigen/src/CholmodSupport/CholmodSupport.h:182:1: error: use of undeclared identifier 'cholmod_free_factor'; did you mean 'M_cholmod_free_factor'?
EIGEN_CHOLMOD_SPECIALIZE1(int, free_factor, cholmod_factor*, L)
^
/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/RcppEigen/include/Eigen/src/CholmodSupport/CholmodSupport.h:177:109: note: expanded from macro 'EIGEN_CHOLMOD_SPECIALIZE1'
    template<typename _StorageIndex> inline ret cm_ ## name       (t1& a1, cholmod_common &Common) { return cholmod_ ## name   (&a1, &Common); }
                                                                                                            ^
<scratch space>:29:1: note: expanded from here
cholmod_free_factor
^
/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/Matrix/include/Matrix/cholmod.h:1105:24: note: 'M_cholmod_free_factor' declared here
R_MATRIX_INLINE    int R_MATRIX_CHOLMOD(free_factor)(
                       ^
/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/Matrix/include/Matrix/cholmod.h:1057:34: note: expanded from macro 'R_MATRIX_CHOLMOD'
#define R_MATRIX_CHOLMOD(_NAME_) M_cholmod_ ## _NAME_
                                 ^
<scratch space>:2:1: note: expanded from here
M_cholmod_free_factor
^
In file included from external.cpp:8:
In file included from ./predModule.h:13:
In file included from ./lme4CholmodDecomposition.h:13:
In file included from /Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/RcppEigen/include/RcppEigenCholmod.h:33:
In file included from /Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/RcppEigen/include/Eigen/CholmodSupport:39:
/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/RcppEigen/include/Eigen/src/CholmodSupport/CholmodSupport.h:183:1: error: use of undeclared identifier 'cholmod_free_dense'; did you mean 'M_cholmod_free_dense'?
EIGEN_CHOLMOD_SPECIALIZE1(int, free_dense,  cholmod_dense*,  X)
^
/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/RcppEigen/include/Eigen/src/CholmodSupport/CholmodSupport.h:177:109: note: expanded from macro 'EIGEN_CHOLMOD_SPECIALIZE1'
    template<typename _StorageIndex> inline ret cm_ ## name       (t1& a1, cholmod_common &Common) { return cholmod_ ## name   (&a1, &Common); }
                                                                                                            ^
<scratch space>:31:1: note: expanded from here
cholmod_free_dense
^
/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/Matrix/include/Matrix/cholmod.h:1103:24: note: 'M_cholmod_free_dense' declared here
R_MATRIX_INLINE    int R_MATRIX_CHOLMOD(free_dense)(
                       ^
/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/Matrix/include/Matrix/cholmod.h:1057:34: note: expanded from macro 'R_MATRIX_CHOLMOD'
#define R_MATRIX_CHOLMOD(_NAME_) M_cholmod_ ## _NAME_
                                 ^
<scratch space>:329:1: note: expanded from here
M_cholmod_free_dense
^
In file included from external.cpp:8:
In file included from ./predModule.h:13:
In file included from ./lme4CholmodDecomposition.h:13:
In file included from /Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/RcppEigen/include/RcppEigenCholmod.h:33:
In file included from /Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/RcppEigen/include/Eigen/CholmodSupport:39:
/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/RcppEigen/include/Eigen/src/CholmodSupport/CholmodSupport.h:184:1: error: use of undeclared identifier 'cholmod_free_sparse'; did you mean 'M_cholmod_free_sparse'?
EIGEN_CHOLMOD_SPECIALIZE1(int, free_sparse, cholmod_sparse*, A)
^
/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/RcppEigen/include/Eigen/src/CholmodSupport/CholmodSupport.h:177:109: note: expanded from macro 'EIGEN_CHOLMOD_SPECIALIZE1'
    template<typename _StorageIndex> inline ret cm_ ## name       (t1& a1, cholmod_common &Common) { return cholmod_ ## name   (&a1, &Common); }
                                                                                                            ^
<scratch space>:33:1: note: expanded from here
cholmod_free_sparse
^
/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/Matrix/include/Matrix/cholmod.h:1107:24: note: 'M_cholmod_free_sparse' declared here
R_MATRIX_INLINE    int R_MATRIX_CHOLMOD(free_sparse)(
                       ^
/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/Matrix/include/Matrix/cholmod.h:1057:34: note: expanded from macro 'R_MATRIX_CHOLMOD'
#define R_MATRIX_CHOLMOD(_NAME_) M_cholmod_ ## _NAME_
                                 ^
<scratch space>:3:1: note: expanded from here
M_cholmod_free_sparse
^
In file included from external.cpp:8:
In file included from ./predModule.h:13:
In file included from ./lme4CholmodDecomposition.h:13:
In file included from /Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/RcppEigen/include/RcppEigenCholmod.h:33:
In file included from /Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/RcppEigen/include/Eigen/CholmodSupport:39:
/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/RcppEigen/include/Eigen/src/CholmodSupport/CholmodSupport.h:186:1: error: use of undeclared identifier 'cholmod_analyze'; did you mean 'M_cholmod_analyze'?
EIGEN_CHOLMOD_SPECIALIZE1(cholmod_factor*, analyze, cholmod_sparse, A)
^
/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/RcppEigen/include/Eigen/src/CholmodSupport/CholmodSupport.h:177:109: note: expanded from macro 'EIGEN_CHOLMOD_SPECIALIZE1'
    template<typename _StorageIndex> inline ret cm_ ## name       (t1& a1, cholmod_common &Common) { return cholmod_ ## name   (&a1, &Common); }
                                                                                                            ^
<scratch space>:35:1: note: expanded from here
cholmod_analyze
^
/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/Matrix/include/Matrix/cholmod.h:1073:24: note: 'M_cholmod_analyze' declared here
R_MATRIX_INLINE CHM_FR R_MATRIX_CHOLMOD(analyze)(
                       ^
/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/Matrix/include/Matrix/cholmod.h:1057:34: note: expanded from macro 'R_MATRIX_CHOLMOD'
#define R_MATRIX_CHOLMOD(_NAME_) M_cholmod_ ## _NAME_
                                 ^
<scratch space>:314:1: note: expanded from here
M_cholmod_analyze
^
In file included from external.cpp:8:
In file included from ./predModule.h:13:
In file included from ./lme4CholmodDecomposition.h:13:
In file included from /Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/RcppEigen/include/RcppEigenCholmod.h:33:
In file included from /Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/RcppEigen/include/Eigen/CholmodSupport:39:
/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/RcppEigen/include/Eigen/src/CholmodSupport/CholmodSupport.h:188:155: error: use of undeclared identifier 'cholmod_solve'; did you mean 'M_cholmod_solve'?
template<typename _StorageIndex> inline cholmod_dense*  cm_solve         (int sys, cholmod_factor& L, cholmod_dense&  B, cholmod_common &Common) { return cholmod_solve     (sys, &L, &B, &Common); }
                                                                                                                                                          ^~~~~~~~~~~~~
                                                                                                                                                          M_cholmod_solve
/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/Matrix/include/Matrix/cholmod.h:1117:24: note: 'M_cholmod_solve' declared here
R_MATRIX_INLINE CHM_DN R_MATRIX_CHOLMOD(solve)(
                       ^
/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/Matrix/include/Matrix/cholmod.h:1057:34: note: expanded from macro 'R_MATRIX_CHOLMOD'
#define R_MATRIX_CHOLMOD(_NAME_) M_cholmod_ ## _NAME_
                                 ^
<scratch space>:8:1: note: expanded from here
M_cholmod_solve
^
In file included from external.cpp:8:
In file included from ./predModule.h:13:
In file included from ./lme4CholmodDecomposition.h:13:
In file included from /Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/RcppEigen/include/RcppEigenCholmod.h:33:
In file included from /Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/RcppEigen/include/Eigen/CholmodSupport:39:
/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/RcppEigen/include/Eigen/src/CholmodSupport/CholmodSupport.h:191:155: error: use of undeclared identifier 'cholmod_spsolve'; did you mean 'M_cholmod_spsolve'?
template<typename _StorageIndex> inline cholmod_sparse* cm_spsolve       (int sys, cholmod_factor& L, cholmod_sparse& B, cholmod_common &Common) { return cholmod_spsolve   (sys, &L, &B, &Common); }
                                                                                                                                                          ^~~~~~~~~~~~~~~
                                                                                                                                                          M_cholmod_spsolve
/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/Matrix/include/Matrix/cholmod.h:1129:24: note: 'M_cholmod_spsolve' declared here
R_MATRIX_INLINE CHM_SP R_MATRIX_CHOLMOD(spsolve)(
                       ^
/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/Matrix/include/Matrix/cholmod.h:1057:34: note: expanded from macro 'R_MATRIX_CHOLMOD'
#define R_MATRIX_CHOLMOD(_NAME_) M_cholmod_ ## _NAME_
                                 ^
<scratch space>:14:1: note: expanded from here
M_cholmod_spsolve
^
8 errors generated.
make: *** [external.o] Error 1
ERROR: compilation failed for package ‘lme4’
* removing ‘/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/lme4’
* restoring previous ‘/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/lme4’
Warning in install.packages :
  installation of package ‘lme4’ had non-zero exit status

The downloaded source packages are in
    ‘/private/var/folders/1d/rjvd0h_n0yq20j13h4gdfytw0000gn/T/Rtmp0KBII7/downloaded_packages’

[eddelbuettel]

Thanks for picking that up! From a first glance, and I could be wrong, I think that the changed @jaganmn brought here in this ticket #131 are of course for the 3.3. branch. The attempt to get to 3.4. was initially done much earlier and may by now be stale. It may take someone with a bit of time and dedication to (if needed) update the 3.4.* changes and 'port' the updates here in #131?

[jaganmn]

I should probably do it ... will try tonight or otherwise this week.