Skip to content

Conversation

mmaechler
Copy link
Contributor

@mmaechler mmaechler commented Mar 13, 2017

For Matrix 1.2-8, I had updated the definition of SparseSuite_long in Matrix/inst/include/cholmod.h,
see (the diffs) here:
https://r-forge.r-project.org/scm/viewvc.php/pkg/Matrix/inst/include/cholmod.h?root=matrix&r1=3040&r2=3200 to avoid problems with oldish / unusual compilers/platforms.... and to make it future proof, notably using 64 bit long on all platforms.
Now this lead to rare (and hard to trace) segmentation faults when using CRAN package lme4 which uses both RcppEigen and classes from Matrix.... reported only in 32-bit Linux here.

What I found (also thanks to Brian Ripley who also found such problems on (32-bit) SPARC Solaris) is that indeed the file inst/include/RcppEigenCholmod.h has originally been a copy-paste-version of the file
Matrix/inst/include/cholmod.h (mentioned above) made almost surely by Doug Bates (@dmbates). And we all forgot that in the end, we use the "same" underlying mapping to Matrix' cholmod and hence must use compatible typedefs.... which we will have again after applying this pull request.

Of course, packages using RcppEigen and which use this header should be recompiled after a new version of RcppEigen is made available on CRAN.
I hope Dirk (@eddelbuettel) can agree to submit that relatively soonish, so it is more or less synchronized with the upcoming new version of the Matrix package (1.2-9) and user would typically install both (when updating).
Also, Matrix 1.2-9 is planned to be in R 3.4.0 (to appear in a bit more than a month I think).

@eddelbuettel
Copy link
Member

Nice detective work. And agreed on the need for common underpinnings for Matrix, lme4 and RcppEigen. [ Aside: Might it make sense to create a common 'base' package with a header all three use? ]

I can make a quick RcppEigen release. We are otherwise trying to get to the next version of Eigen in RcppEigen but that work is mostly done by @yixuan.

Copy link
Member

@eddelbuettel eddelbuettel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I can run a rev.dep check tomorrow (as I currently run one for Rcpp).

@mmaechler
Copy link
Contributor Author

mmaechler commented Mar 13, 2017

Good, thank you Dirk!
Apropos common 'base' package: Fine in theory but probably too complicated by the fact that 'Matrix' has special status "Recommended".. so the "base" would have to be "Recommended" too.
I think Matrix itself might serve as "base" here, if RcppEigen was "LinkingTo: Matrix" and I don't know if I ever knew why Doug (or you?) rather wanted to "Cut 'n Paste" there...

@eddelbuettel
Copy link
Member

Good point -- so in that case a common header defining this, placed in either Matrix and lme4, would indeed help as can essentially "always" assume for them to be present.

@eddelbuettel eddelbuettel merged commit 7bc0efb into RcppCore:master Mar 14, 2017
@eddelbuettel
Copy link
Member

@mmaechler I have the package ready (after adding src/init.c and other little changes).

When do you want this uploaded?

@mmaechler
Copy link
Contributor Author

Well, I've submitted Matrix_1.2-9 to CRAN this morning (MET) and remarked that the segfault seen in lme4 on 32-bit platforms would be solved by you releasing a new version of RcppEigen after your reverse dependency checks have completed.
So, the answer to "when ?" would be "ASAP". It is also fine if the new RcppEigen hits CRAN before the new Matrix, as the problem has also affected the current CRAN version of Matrix (= 1.2-8).

Thanks a lot for your swift reactions on this issue!

@eddelbuettel
Copy link
Member

Perfect. Running a rev.dep check on the 91 reverse depends, currently at 72 all of which are (of course) still good so I will follow later.

I guess I just misread what you wrote earlier, not getting a the "ASAP" but rather implying some later, coordinated attempt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants