Correct options on Eigen::MappedSparseMatrix. #3343
Conversation
The only accepted `Options` for `SparseMatrix` are `RowMajor` and `ColMajor`. Note that `Options` != `Flags`, so passing `Type::Flags` leads to invalid options. This is checked in the latest version of Eigen via a static assertion, which causes a build break.
|
We should bump the Eigen version here as well: Line 179 in f4c81e0 |
|
Looks like (a) problem is: https://gitlab.com/libeigen/eigen/-/blob/3.4.0/Eigen/src/Core/PlainObjectBase.h#L177 - that's a constant expression, so MSVC warns about it. That's really not our fault. I'm guessing SYSTEM includes don't help MSVC. |
|
Should we just add |
|
If it could be localized enough. Ideally on the Eigen include, if MSVC was happy with that (sometimes it has to be at usage, not at include). |
Sounds good to me. I think that's probably the best way to handle this particular situation. |
|
@rwgk Since you are most familiar with these pragmas, would you mind giving a stab at it? |
I'll give it a shot. |
| set(PYBIND11_EIGEN_VERSION | ||
| "dc252fbf00079ccab57948a164b1421703fe4361" | ||
| "929bc0e191d0927b1735b9a1ddc0e8b77e3a25ec" |
There was a problem hiding this comment.
| "929bc0e191d0927b1735b9a1ddc0e8b77e3a25ec" | |
| "05c9d7ce201bfcbe7a2706bef4f893038ae251e5" |
I cherry-picked disabling the MSVC warning from the master branch, which should only disable the warning locally within Eigen.
There was a problem hiding this comment.
Thoughts @rwgk ? I think it would be better if we pointed to a stable eigen version especially for downstream usecases.
There was a problem hiding this comment.
Sorry, I'm uncertain what this means, could you please give more context?
When did you cherry pick? From the Eigen master?
There was a problem hiding this comment.
@rwgk They fixed the warning on Eigen Master, but it's not in a new stable version yet.
There was a problem hiding this comment.
Could we just wait for the 3.4.1 release then (and maybe document somewhere that Eigen 3.4.0 (only) has the C4127 warning issue)?
There was a problem hiding this comment.
Given the state of Eigen's repo right now, that might be the best course of action.
There was a problem hiding this comment.
I decided to try this anyway (CI just triggered, waiting for results): #3352
See the added comment in eigen.h for the rationale.
There was a problem hiding this comment.
@rwgk I cherry-picked the change disabling the MSVC warning from Eigen's master branch onto Eigen's 3.4 branch. Someone else modified this PR to pull from the tip of the 3.4 branch, which is not the 3.4.0 release (i.e. commit 929bc0e191d0927b1735b9a1ddc0e8b77e3a25ec).
The Eigen update/warning issue is incidental to the original PR, which is to correct the options on MappedSparseMatrix, and is incorrect regardless of which version of Eigen is used - it's just that on the master branch, this currently fails to build because of new static assertions.
This is blocking me from updating Eigen internally at Google, since we sync with the Eigen master branch (of which I am a maintainer). If updating the version of Eigen used here is causing other issues, then can we file that as a separate issue and address later? I would appreciate getting the original change in as soon as possible.
There was a problem hiding this comment.
Got it, thanks.
Ideally we want a bit cleaner CI though to be more certain we're not overlooking something.
I'll pull your original change into my PR for testing.
…o not run past 99 columns (our desired but currently not enforced limit).
…ression around Eigen includes. (#3352) * Adding MSVC C4127 suppression around Eigen includes. * For MSVC 2015 only: also adding the C4127 suppression to test_eigen.cpp * Copying original change from PR #3343, with extra line breaks to not run past 99 columns (our desired but currently not enforced limit).
|
Original change merged via PR #3352. |
Description
The only accepted
OptionsforSparseMatrixareRowMajorandColMajor.Note that
Options!=Flags, so passingType::Flagsleads to invalidoptions. This is checked in the latest version of Eigen via a static
assertion, which causes a build break.