Header clean-up to fix lme4/lme4#745 #131
Conversation
There was a problem hiding this comment.
This is a pretty big PR, and it goes pretty deep into internals Doug once set up. But if there is a person I would trust with that it is probably you.
I will run these through reverse dependency check which will tell us more, but this looks like a very good simplification / disentanglement of some hairy internals.
| Imports: Matrix (>= 1.1-0), Rcpp (>= 0.11.0), stats, utils | ||
| Suggests: inline, tinytest, pkgKitten, microbenchmark | ||
| Imports: Rcpp (>= 0.11.0), stats, utils | ||
| Suggests: Matrix, inline, microbenchmark, pkgKitten, tinytest |
There was a problem hiding this comment.
Just a side comment and not a request to change: When I modify code (particular "other people's code" I try to be minimal. Adding Matrix was needed on the Suggests:, doing it at the front is fine, reordering the four remaining words just puts cognitive load here that we do not need. (Again, no need to change, just a comment.)
| #include <Matrix.h> | ||
| #ifndef R_MATRIX_CHOLMOD /* Matrix <= 1.6-1.1 */ | ||
| # define R_MATRIX_CHOLMOD(_NAME_) M_cholmod_ ## _NAME_ | ||
| # define M_cholmod_start M_R_cholmod_start /* sigh */ |
There was a problem hiding this comment.
(Just a question: Remind again what that second line does / why we need it / why the sigh? )
There was a problem hiding this comment.
Matrix <= 1.6-1.1 uses the M_ prefix for all of its stubs except for cholmod_start, where it uses the M_R_ prefix. 1.6-2 uses M_ everywhere and preserves backwards compatibility with the same #define (but reversed). A minor nuisance ...
There was a problem hiding this comment.
Ahhhh. A local fix. I see now!
|
Thanks - I expect lme4 to break, but I'll be curious about which others ... |
|
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 is provided by lme4 and not by Eigen. Compare: https://github.com/jaganmn/lme4/blob/rcppeigen_cholmod/src/lme4CholmodDecomposition.h#L20-L73 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. |
|
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 The reverse dependencies started to run before you added your third commit here. I have now updated the package and installed that updated package. |
|
Thanks, Dirk. The patched version of lme4 should pass its checks. I'll take a look at OpenMx, hopefully today. |
|
One question though: could or should the 'stub' be a header fiile (likely with |
and then add I guess the file extension is ultimately up to you. If you decide on |
|
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. |
|
Bump. I've reverted the change to |
|
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 ...) |
|
Passed pre-test at CRAN and got the email that automated reverse dependency checks have been launched. |
|
@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. |
|
@bbolker I presume you followed up to Uwe's email and sent an |
|
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. |
|
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. |
|
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 |
|
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' ? |
|
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. |
|
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. |
|
I've been re-running the reverse-dependency checks for the Eigen 3.4.0 update, and these changes are causing failures with Is this something that would need fixing in |
|
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? |
|
I should probably do it ... will try tonight or otherwise this week. |
port RcppCore#131, adapted to Eigen 3.4.0
* import Eigen 3.4.0 * remove cholmod_l_* functions * apply patches * diffs * changelog * mark as release candidate while we test * port #131, adapted to Eigen 3.4.0 * revert changes below inst/include/Eigen in merge commit 4db13cb * Tweak to build with clang on Windows. * Update ChangeLog * RcppEigen 0.3.4.0.0 --------- Co-authored-by: Dirk Eddelbuettel <edd@debian.org> Co-authored-by: Mikael Jagan <jaganmn@mcmaster.ca> Co-authored-by: Tomas Kalibera <tomas.kalibera@gmail.com>
I would summarize the changes as follows:
RcppEigen.h.RcppEigenCholmod.handRcppEigenStubs.cpp(renamed from*.h) to use Matrix API, not relying on static copies. Packages must includeRcppEigenCholmod.hin addition toRcppEigen.hto enable CHOLMOD support. They must compileRcppEigenStubs.cppif includingRcppEigenCholmod.h.DESCRIPTIONandNAMESPACE. Packages includingRcppEigenCholmod.hmust haveLinkingTo: RcppEigen, Matrix.Note that it was necessary to modify two files under
inst/include/Eigendirectly, mainly because the Matrix API prefixes its CHOLMOD stubs.