Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eigen 3.4.0 #102

Merged
merged 16 commits into from
Feb 29, 2024
Merged

Eigen 3.4.0 #102

merged 16 commits into from
Feb 29, 2024

Conversation

yixuan
Copy link
Collaborator

@yixuan yixuan commented Oct 26, 2021

This PR should have solved the issue mentioned in #101.

The problem was that in Eigen 3.4.0, Eigen/src/CholmodSupport/CholmodSupport.h introduced a series of cholmod_l_* functions intended for the long integer type, which were not defined in RcppEigenCholmod.h. The fix is to remove the definitions of those functions, as they are not used by RcppEigen.

Note that I have only tested the building of RcppEigen itself and that of my own RSpectra package. I expect that there may be some issues for other packages depending on RcppEigen. We still need @eddelbuettel to schedule a (large-scale) automated test for the huge number of dependent packages.

@eddelbuettel
Copy link
Member

Perfect. I started to poke there too, but I only altered them to get to subsequent template errors, and then it got late.

@eddelbuettel
Copy link
Member

BTW you are a co-author so I am sure you have write permissions in this repo so you could and (maybe should) just branch here. I will put a trivial patch back to you call this 0.3.3.99.0 for now to mark it as a (unrelease) release candidate while we test.

@yixuan
Copy link
Collaborator Author

yixuan commented Oct 26, 2021

Oh that's good. I didn't realize that I have write permissions. Thanks for reminding.

@eddelbuettel
Copy link
Member

My bad, you didn't, but should have. Rectified now, invite in your inbox.

There is actually a Rcpp reverse depends check running now, and those tend to take a moment. RcppEigen will be next; we may know more later today or tomorrow.

@bbolker
Copy link

bbolker commented Oct 26, 2021

FWIW looks like lme4 passes with new version (after one tolerance adjustment, will require new version on CRAN ...) Apparently a slight slowdown: 8m38.904s for check with 0.3.3.9.1, 9m33.881s with new version. Haven't done any more careful benchmarking. Slowdown would be too bad (and a bit of a pain for CRAN submission, we might have to skip more tests on CRAN), but not the end of the world.

@eddelbuettel
Copy link
Member

Where did you test that? 'Performance regression testing' is known to be impossible on CI frameworks as we never know what hardware we're on each time -- or was it your own machine?

In any event I am "merely the messenger" 😀 -- the delta is all upstream.

@bbolker
Copy link

bbolker commented Oct 26, 2021

It was my own machine. I'm not terribly concerned about this, just reporting my experience.

@eddelbuettel
Copy link
Member

So it took a moment before the Rcpp reverse depends check complete, and the one for RcppEigen is still running ... and it brings good and bad news. In short, just like the change to 3.3.0 there is a bit of disruption. A few packages no longer build, and if we want 3.4.0 in R, we need to buckle up and adjust the packages (not unlike what I have been doing for Rcpp over here). I'll open a ticket here.

@waynelapierre
Copy link

Any updates on this PR?

@eddelbuettel
Copy link
Member

Please see #103 and help as you can.

@eddelbuettel
Copy link
Member

I think I got the merge conflicts sorted the right way --- but it might benefit from another glance or two by @jaganmn at the merge commit 4db13cb .

@andrjohns
Copy link

@eddelbuettel I re-ran the revdep checks with this branch and lme4 and OpenMx both pass now (thanks for sorting that so quickly @jaganmn!).

Down to 8 packages on CRAN that break, with three having already merged patches: #103 (comment)

@jaganmn
Copy link
Contributor

jaganmn commented Jan 19, 2024

I left one comment on Dirk's merge commit, but apart from that it seems OK.

@eddelbuettel
Copy link
Member

Just replied to @jaganmn there and in short I was just trying to move the PR over a hump of a merge conflict. I may have done that in haste.

@eddelbuettel
Copy link
Member

Lovely stuff. Thank you all -- will turn on a rev.dep run here too.

@eddelbuettel
Copy link
Member

It has been shipped to CRAN following all the reverse-dependency work chronicled in #103 !!

One question: if/when this passes as we hope, how do we merge this? If we do a merge commit we get really ugly (but truthful) parallel histories in git ls (in 'graph mode'). Maybe despite the ugliness that is best to show the full history?

Else we could squash merge for linear history (which I like in other repos) but it brushed a lot under the carpet. Thoughts? Preferences? Or non-issue?

@eddelbuettel
Copy link
Member

Thanks, on its way to CRAN.

Yay!

Thoughts on how to merge as per my previous comment?

@yixuan
Copy link
Collaborator Author

yixuan commented Feb 29, 2024

This is fantastic!!

I really have no preference on the merging strategy here. But if I have to make a choice, I may vote for the cleaner way. To be honest, I was always puzzled by git histories with multiple sources of changes. 🤣

Other thoughts?

@eddelbuettel
Copy link
Member

Hey -- good to hear from you. I am really torn. I like 'linear' -- at work we enforce it via 'squash merge': each PR is a squash.

image

I don't mind standard PR with (usually) just one departure:

image

I am less of a fan where it gets 'wild'

image

Because yours started so long ago we'd have a really long parallel history. But then again we a) have never squashed and b) it's nice if your work is reflected as is.

So kinda leaning towards normal 'noisy' merge. What do you think?

[ But most importantly: it does not really matter. Code quality matters, clean tests, good ci. This is mostly cosmetic. It matters really only a little bit. ]

@yixuan
Copy link
Collaborator Author

yixuan commented Feb 29, 2024

Hi Dirk, if the consideration is on reflecting "my work" then I really do not mind squashing it. The changes mostly come from upstream Eigen, and I would not take that much credit. 😄

@eddelbuettel
Copy link
Member

Ok -- squash it is then! (I switched in Rcpp too. It is cleaner. Will take a while til we have a 'page full' though )

@eddelbuettel eddelbuettel merged commit b92b5b2 into RcppCore:master Feb 29, 2024
3 checks passed
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.

7 participants