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.

Sign up for GitHub

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

Correct options on Eigen::MappedSparseMatrix. #3343

Closed
wants to merge 3 commits into from
Closed

Correct options on Eigen::MappedSparseMatrix. #3343

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
101b42e
Correct options on Eigen::MappedSparseMatrix.
cantonios Oct 8, 2021
05fbd92
Update Eigen to 3.4
Skylion007 Oct 11, 2021
6e42c94
Update version
Skylion007 Oct 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/pybind11/eigen.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -559,7 +559,7 @@ struct type_caster<Type, enable_if_t<is_eigen_sparse<Type>::value>> {
if (!values || !innerIndices || !outerIndices)
return false;

value = Eigen::MappedSparseMatrix<Scalar, Type::Flags, StorageIndex>(
value = Eigen::MappedSparseMatrix<Scalar, Type::Flags & (Eigen::RowMajor | Eigen::ColMajor), StorageIndex>(
shape[0].cast<Index>(), shape[1].cast<Index>(), nnz,
outerIndices.mutable_data(), innerIndices.mutable_data(), values.mutable_data());

Expand Down
4 changes: 2 additions & 2 deletions tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,9 +174,9 @@ set(PYBIND11_CROSS_MODULE_GIL_TESTS test_gil_scoped.py)
set(PYBIND11_EIGEN_REPO
"https://gitlab.com/libeigen/eigen.git"
CACHE STRING "Eigen repository to use for tests")
# This hash is for 3.3.8, using a hash for security reasons
# This hash is for 3.4.0, using a hash for security reasons
set(PYBIND11_EIGEN_VERSION
"dc252fbf00079ccab57948a164b1421703fe4361"
"929bc0e191d0927b1735b9a1ddc0e8b77e3a25ec"
Copy link
Author

Choose a reason for hiding this comment

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

Suggested change
"929bc0e191d0927b1735b9a1ddc0e8b77e3a25ec"
"05c9d7ce201bfcbe7a2706bef4f893038ae251e5"

I cherry-picked disabling the MSVC warning from the master branch, which should only disable the warning locally within Eigen.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thoughts @rwgk ? I think it would be better if we pointed to a stable eigen version especially for downstream usecases.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I'm uncertain what this means, could you please give more context?
When did you cherry pick? From the Eigen master?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@rwgk They fixed the warning on Eigen Master, but it's not in a new stable version yet.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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)?

Copy link
Collaborator

@Skylion007 Skylion007 Oct 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Given the state of Eigen's repo right now, that might be the best course of action.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I decided to try this anyway (CI just triggered, waiting for results): #3352

See the added comment in eigen.h for the rationale.

Copy link
Author

Choose a reason for hiding this comment

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

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

CACHE STRING "Eigen version to use for tests")

# Check if Eigen is available; if not, remove from PYBIND11_TEST_FILES (but
Expand Down