New user self-registration is disabled due to spam. Please email eigen-core-team @ lists.tuxfamily.org if you need an account.
Before reporting a bug, please make sure that your Eigen version is up-to-date!
Bug 422 - Reconsider EIGEN_DEFAULT_TO_ROW_MAJOR
Summary: Reconsider EIGEN_DEFAULT_TO_ROW_MAJOR
Status: REOPENED
Alias: None
Product: Eigen
Classification: Unclassified
Component: Core - general (show other bugs)
Version: 3.0
Hardware: All All
: Normal Unknown
Assignee: Nobody
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 4.0
  Show dependency treegraph
 
Reported: 2012-02-22 13:43 UTC by Hordur Johannsson
Modified: 2012-06-06 15:40 UTC (History)
5 users (show)



Attachments
Example that fails linking (2.12 KB, application/x-gzip)
2012-02-22 13:43 UTC, Hordur Johannsson
no flags Details

Description Hordur Johannsson 2012-02-22 13:43:34 UTC
Created attachment 254 [details]
Example that fails linking

I recently came across a potential problem caused by providing the EIGEN_DEFAULT_TO_ROW_MAJOR define.

The problem is when you are using libraries built with different options then there can be problems during linking because MatrixXd  will have a different types depending on whether -DEIGEN_DEFAULT_TO_ROW_MAJOR was used or not.
This makes it hard to use libraries compiled with different options in the
same programs. Because when you include headers with MatrixXd, the type
doesn't reflect the type when the library was built.

I've attached a minimal example with program c.cpp that uses libraries a.cpp and b.cpp that are compiled with and without -DEIGEN_DEFAULT_TO_ROW_MAJOR. This causes c.cpp failing to compile, because Eigen::MatrixXd isn't the same type in a.o and b.o.

If someone runs into this problem there are few options:
  1. Re-compile library b.cpp with the -DEIGEN_DEFAULT_TO_ROW_MAJOR
  2. Modify the header files for library b to 
     void b(Matrix<double, Dynamic, Dynamic, ColMajor> M);
  3. Re-organize my code so that I do not include a.h and b.h 
     in the same compilation unit.

Option 1. is probably easiest but sometimes one uses pre-packaged libraries, e.g. Ubuntu packages.
Option 2. would be good, if library developers ensured to be explicit on 
ordering on their public interfaces. By not providing 
EIGEN_DEFAULT_TO_ROW_MAJOR means that MatrixXd has an explicit ColMajor 
ordering.

I see the usefulness with having the EIGEN_DEFAULT_TO_ROW_MAJOR, but I'm worried that it can lead to incompatibilities. 

Is there some better way to handle this?
Comment 1 Benoit Jacob 2012-02-22 13:56:00 UTC
Some options do break ABI compatibility. EIGEN_DEFAULT_TO_ROW_MAJOR is one of them, but not the only one. Another is EIGEN_DONT_ALIGN_STATICALLY, for example. I think that this is a documentation/communication issue. These features are very useful to some people who know what they're doing but clearly they should not be used in code that needs to link with arbitrary other Eigen-using code.
Comment 2 Christoph Hertzberg 2012-02-22 14:10:45 UTC
Actually, I never saw a really convincing argument for EIGEN_DEFAULT_TO_ROW_MAJOR.
If you need to interface with another library, which uses row-major matrices, I would say you need to write Matrix<double, Dynamic, Dynamic, RowMajor>::Map(...) etc anyways, or you need to assert that EDTRM is defined.

Also, whenever you use .data() or Map or initialize (fixed size) matrices using a scalar pointer, you need to explicitly specify the intended order of your matrix, otherwise you get different behavior depending on the compile options, i.e. EDTRM even breaks API compatibility!

One single case I can think of, where EDTRM could be useful is if an algorithm happens to be faster/better vectorizable in row-major mode (and the implementor is to lazy to add that option each time, or just wants to quickly check if it really is faster).

I'm afraid it is too late to disallow the macro, though ...
Comment 3 Radu B. Rusu 2012-02-22 17:42:18 UTC
Agreed - we got a bit of a mess with our own library (pointclouds.org) for trying to be too clever and support both row major and column major matrices. As Cristoph mentioned, the fact that EDTRM breaks _API compatibility_ is huge. Basically a core typedef that you were relying on to be something (e.g., MatrixXf) is now something completely different.

I would have also liked to see a comparison / performance test of all Eigen operators in column vs row major to see what the differences in performance are.
Comment 4 Gael Guennebaud 2012-02-22 19:37:39 UTC
I must say that I dislike this EIGEN_DEFAULT_TO_ROW_MAJOR. From my point of view this should only be an internal trick to be used by our unit tests.
Comment 5 Christoph Hertzberg 2012-02-22 22:49:56 UTC
If everyone agrees that the macro is mostly a hack used for testing, how about deprecating the macro (i.e. let it raise a warning explaining the issue) and replace it by something like EIGEN_DEFAULT_TO_ROW_MAJOR_YES_I_KNOW_THIS_BREAKS_ABI_AND_API_COMPATIBILITY?
(I deliberately exaggerated with the length of the new identifier)
I think no "normal" user will use the macro anyways, and "pro"-users should be able to adapt their compile flags and insert RowMajor template parameters where required.

I admit that unit-tests are one case where this flag /might/ be useful, but as I understand it, this requires unit-testers to compile and run both w/ and w/o the flag? Or does the testing framework do this automatically somehow?
Otherwise it might be better to automatically run tests both for Column- and RowMajor (maybe in the same spirit as e.g. test/product_trmm.cpp does it already, i.e. also for all combinations)
Comment 6 Christoph Hertzberg 2012-02-24 15:26:29 UTC
This should be decided before releasing 3.1. If it's a WONTFIX it should at least be reconsidered in the far future of Eigen 4.0.
Comment 7 Jitse Niesen 2012-05-13 22:46:38 UTC
For the moment, I moved the EIGEN_DEFAULT_TO_ROW_MAJOR macro to the developer-only section. I also added a warning that macros can break ABI/API and that that is dangerous. Changeset 08792adb00d6.
Comment 8 Gael Guennebaud 2012-06-06 15:36:08 UTC
It should be clear now that this macro should not be considered by user code. For the existing code, well that's too late already. I'll add an entry for 4.0.

Note You need to log in before you can comment on or make changes to this bug.