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

Workaround for #3140 Compilation error using newer Eigen library. #3142

Closed
wants to merge 2 commits into from

Conversation

arianepaola
Copy link
Contributor

This pull request is a workaround for #3140 causing a compilation error, when using Eigen 3.3 versions.
It adds an additional check to verify that the Eigen version is between 3.1.0 and 3.2.5. (excluding >= 3.2.6)

@vigsterkr
Copy link
Member

@arianepaola good, but would be better if we could actually solve this by actually have a solution for 3.3+ :) because now as far as i understand this will just not use eigen 3.3+, or?

@@ -11,7 +11,7 @@

#include <shogun/mathematics/eigen3.h>

#if EIGEN_VERSION_AT_LEAST(3,1,0)
#if EIGEN_VERSION_AT_LEAST(3,1,0) && (EIGEN_WORLD_VERSION == 3 && EIGEN_MAJOR_VERSION == 2 && EIGEN_MINOR_VERSION < 6 )
Copy link
Member

Choose a reason for hiding this comment

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

Can you use the CMake function VERSION_LESS
See e.g. examples/meta/cpp/CMakeLists.txt
It is a bit cleaner

@karlnapf
Copy link
Member

karlnapf commented Apr 1, 2016

I agree with @vigsterkr on fixing this somehow rather than excluding new eigen versions.
However, I am fine to merge this as a temp. solution as we can just bundle eigen if the system version is too recent. I leave it to @vigsterkr to decide

@vigsterkr
Copy link
Member

@karlnapf no temp merge. i hate temp merges 💃

@vigsterkr
Copy link
Member

i mean i really appreciate the effort here! i do! but let's try to fix it so that 3.3+ can play as well. should not be that hard

@vigsterkr
Copy link
Member

btw i should have written after the 💃 caramba!

@karlnapf
Copy link
Member

karlnapf commented Apr 1, 2016

hahahah ;)

@karlnapf
Copy link
Member

karlnapf commented Apr 1, 2016

I would merge this one. Requiring a stable eigen version (and bundling if not available) is fine in my eyes. Once the new eigen is out, we can change that

@vigsterkr
Copy link
Member

CARAMBA! 🚯
be more patient :) i'll add patches here and then let's merge it plz.

@arianepaola
Copy link
Contributor Author

@vigsterkr @karlnapf Thanks for the comments, they make sense.
But as an example Ubuntu 16.04 will be released this month and it is usual to release with beta packages. Unfortunately the Eigen 3.3 beta also uses a 3.2.x version numbering.

I have modified my pull request to take out the version numbers from the source file and to rely only on one preprocessor definition. This moves the minimum Eigen version to a preprocessor definition and also resolves a mismatch between the source files (e.g. at least 3.1.0) and the CMake required Eigen 3.1.2.
Now EIGEN_VERSION_DEPENDABLE surrounds the previous code block.

It is not a good idea to force the download of earlier Eigen versions, such as the defined 3.2.8 in cmake/external/Eigen3.cmake as it will hide bugs such as #3140 and will hinder testing shogun on newer versions.

The modified pull request will still trigger the error in #3140 and provide a temporary work around for #3142.

After providing a final work around for #3142, the CMake IF clause can be modified to just use at least Eigen 3.1.2.

Thanks, Ariane

@vigsterkr vigsterkr closed this Feb 28, 2017
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.

None yet

3 participants