#41 Merged at 84e1912
Repository
yoco
Branch
default
Repository
eigen
Branch
default
Author
  1. 蕭刻庭
Reviewers
Description

Implement reshape function:

MatrixXd m(3, 2);
m << 1, 2,
     3, 4,
     5, 6;
MatrixXd n = m.reshape<3, 2>();
std::cout << n << std::endl;

output:

1 5 4
3 2 6

According to http://eigen.tuxfamily.org/bz/show_bug.cgi?id=437&GoAheadAndLogIn=1. I followed Christoph's suggestion, implement a basic support for reshape. Documentation added, too.

There is one specialization template for random access I did not implement yet, because currently I have no idea what that is.

I am new to the eigen project, probably I have made several mistakes in my pull request. Please review my code and give my suggestion. Thank you all.

Comments (15)

  1. Christoph Hertzberg

    Very nice work! Some comments (I only glanced over it, so more things might follow): <!-- Edit: only fixed formatting -->

    • We want to keep the core functionality of Eigen C++03 compatible as long as possible, so please avoid auto and {..,..} initializations. You could make a typedef for the std::pair<Index,Index> to keep things brief.
    • I think for RowMajor expressions the reshape operation should be interpreted as a RowMajor operation as well (basically swap the interpretation of rows and cols in index_remap). Actually, this raises a question, what should happen to expressions like (X + X.transpose()).reshape(...) (no problem on your side -- reshape simply inherits the Majornes from its expression).
    • A unit test would be appreciated (E.g. comparing Map(X.eval().data(), rows, cols) to X.reshape(rows, cols) for various X).
    • You can add your name to the copyright section at the top.
  2. 蕭刻庭 author

    Thank you very much for reviewing my pull request. I have followed you suggestion, done the two issue:

    • remove c++11 feature, make it c++98/03 compatible
    • add unit-test for reshape (found a bug in fixed-size reshape, and fixed)

    And I have a question about the "RowMajor" issue you mentioned.

    I copy the Block.h to Reshape.h, modified all (row, col) calculations. Those transformed (row, col)s are passed to functions like coeff(), coeffRef() or const casted const_cast_derived().coeff(). Though I am not sure, but I guess those coeff() functions will take care of the row/col major issue, because the Block.h dose not do anything special to those transformed (row, col). Do I misunderstood anything?

    Thank you again.

  3. Jitse Niesen

    Very good, this functionality a common request so nice to see some action on it. I think it's fine to not implement direct access at the moment (but Map seems to be the way to go). However, you seem to claim direct access in the traits of Reshape if XprType has DirectAccessBit set, which does not seem right to me. Potentially related to this is that I get a segmentation fault with the following code:

    MatrixXd A = MatrixXd::Identity(4,4);
    MatrixXd B = A.reshape(8,2).leftCols(2);
    

    I want to look a bit more at the traits; there are some things seemed dubious to me at first sight (too much was copied over?) but this is tricky business.

    Am I correct in thinking that InnerPanel is always false? And the Reshape::Reshape(XprType, Index) "column or row constructor" seems to be never called; same for ReshapeImpl.

    In reply to Christoph's question, I seem to remember that if you mix row-major and column-major expressions, the result is column major (or whatever the default storage order is). But as he said, it is not a problem for you. The coeff() function indeed takes care of this.

  4. 蕭刻庭 author

    Thank you for the reviewing. Yes, all of the code are copied from Block.h, then modified. I only understand a small part of it. I am sorry I can not answer the question about the InnerPanel, since I don't know what InnerPanel for. I should dive deeper to figure out every argument/function in this code, then try to simplify it. Thank you for finding the DirectAccessBit bug, I will try to fix it, and add more test case.

  5. 蕭刻庭 author

    Hi Jiste. After trace the code and googled some document, I fixed all the issue you have mentioned.

    And trying to further simplify the those flags.

  6. 蕭刻庭 author

    Hi all, I have several question while simplify the flag enum.

    1. is it legal that (Max)RowsAtCompileTime or (Max)ColsAtCompileTime be 0?

    2. I Thought the IsRowMajor can be directly set the same as the XprType, but when I do so, the compiler complain about my m.reshape<1, 16>(). m is a Matrix4i, it hit a static-assertion in PlainObjectBase.h:712. According to the static-assertion, when MaxRowsAtCompileTime is 1 and MaxColsAtCompileTime is 0, it has to be row major. However, if I have a Matrix4i, and static reshape<1, 16>, though the static row size is 1, but the memory is total the same with the not-reshaped one, why we should treat that reshaped result as row major?

    I think I am still lack a lot of background knowledge and design principle of eigen. Is there are a forum for develop or something like that? Thanks again.

  7. Jitse Niesen

    I am afraid the whole design, especially the internal parts, is not described clearly (one source of information is http://eigen.tuxfamily.org/dox-devel/group__flags.html ; apart from that you need to read the code or ask). Moreover, it is liable to change in an upcoming rewrite of the whole expression template mechanism.

    It is legal to have 0 rows or 0 columns, so this should be supported.

    Per the static assertion, we assume that row vectors are row major and column vectors are column major (here, the definition of row/column vector is as in the assertion; it is a Matrix which is known at compile time to be a vector). This is because the storage order does not matter for vectors. It is silly (and costly, in terms of compile time and executable size) to have two different types that act in precisely the same manner.

    For reshape, that means that when the user reshapes a matrix to a vector, you need to change the storage order to conform to the convention. Conversely, when the user reshapes a vector to a matrix, we need to decide what to do with the storage order. Keeping it the same seems logical, but does have strange consequences: if m is a Matrix4i (column major by default), as you say, then m.reshape<1,16>() is a row vector so should be row major, but then m.reshape<1,16>().reshape<4,4>() is also row major, even though one expects this to be a no-op. Alternatively, we could revert to default storage order when reshaping a vector to a matrix, but then the sequence of reshapes changes storage order if you start with a row major Matrix4i. Perhaps even worse is that the storage order of the result changes if you use dynamic instead of fixed size matrices. Yet another alternative would be to allow the user to specify the storage order of the result.

    Perhaps this suggests that our convention (enforced by the static assertion) needs to be looked at again, but for the moment I suggest you do as the static assertion says.

    1. Christoph Hertzberg

      <offtopic>To be honest, I never was really convinced by the row/col-vectors must be RowMajor/ColMajor argument. I doubt it has a significant impact on compile-time and executable size, especially if we wrap all "working" functions, so that both types essentially call the same. Furthermore, I don't think accidentally mixing Row and ColMajor vectors will happen that often -- and if it is intended, we should hope the user knows what he's doing. On the other hand, it makes writing generic code more complicated (say you need a NxM ColMajor matrix, where N in some cases is 1. Btw: Nx0 and 0xN matrices are allowed to be row or column major.</offtopic>

      To solve the reshape ambiguity: How about always reshape in a Column-Major sense and either add another template parameter/different function for row-major reshaping, or require the user to write X.transpose().reshape(...).transpose() if RowMajor reshaping is wanted?

  8. 蕭刻庭 author

    Hi. everyone ^^ I am still working on reshape.

    But I really hit the wall and need some help. While writing test case

    #include <Eigen/Dense>
    
    int main()
    {
        Eigen::Matrix4i A = Eigen::Matrix4i::Random(4, 4);
    
        A.reshape<1, 16>() // row major
         .reshape<16, 1>() // col major
         ;
    }
    

    I hit a static_assertion at Eigen/src/Core/DenseBase.h:537

      EIGEN_STATIC_ASSERT((EIGEN_IMPLIES(MaxRowsAtCompileTime==1 && MaxColsAtCompileTime!=1, int(IsRowMajor))
                        && EIGEN_IMPLIES(MaxColsAtCompileTime==1 && MaxRowsAtCompileTime!=1, int(!IsRowMajor))),
                          INVALID_STORAGE_ORDER_FOR_THIS_VECTOR_EXPRESSION)
    

    I found I was blocked by the second line (the col-major case). The row is 1, and col is not 1, should be a column major vector, but the IsRowMajor is true.

    The most likely problem is I miss calculated the flags of the reshape-expression-class, but I checked the it several times, and can't not found the error. I had tried to move this static assert to Reshape.h, and the static assertion passed?! (what ...!) I checked the flags in both Eigen/src/Core/DenseBase.h and Reshape.h and can found no error.

    Does anybody has any suggestion to locate that problem? Yes I am asking some one else to debug for my code :( I know it is very bad, but I had tried several days, traced codes, and I really want eigen has the Reshape function.

    Thanks in advance.

    Please help me

  9. Gael Guennebaud

    I don't know if that fix your precise issue, but the MaxRowsAtCompileTime (resp. MaxColsAtCompileTime) should be Dynamic if MatrixRows (resp. MatrixCols) is Dynamic. You can only guarantee that MaxSizeAtCompileTime does not increase.

  10. Gael Guennebaud

    The {Inner/Outer}StrideAtCompileTime are also wrong, but that only concerns the direct-access use case.

  11. 蕭刻庭 author

    Hi, all.

    Finally I found the stupid bug in flag calculation.

    The reshape is basicly "usable".

    However, there are still two unresolved issue:

    1. the majority-lost problem.

    Eigen::Matrix4i A = Eigen::Matrix4i::Random(4, 4); 
    std::cout << A.IsRowMajor << std::endl;  // col major
    std::cout << A.reshape<1, 16>().reshape<4, 4>().IsRowMajor << std::endl;  // row major
    

    when static reshape to <1, 16>, the matrix degrade to a row vector, and lost it original majority, even reshape back to its original shape.

    I had tried add some trick like

    template<typename NestedXprType, int ReshapeRows, int ReshapeCols, int ColNested>
    struct traits<Reshape<Reshape<NestedXprType, 1, ColNested>, ReshapeRows, ReshapeCols> > : traits<Reshape<NestedXprType, 1, ColNested> >
    

    but it still can not cover all cases, for example:

    A.reshape<1, 16>().reshape<1, 16>().reshape<4, 4>();
    A.block<1, 4>().reshape<2, 2>();
    

    My solution is ugly and broken for most of all case, so I draw back it.

    2. the direct-access of a reshaped block

    After read some code and docs, if I have not misunderstood, the inner/outer stride is used to calculate the pitch of memory address while iterating a matrix. InnerStride is the memory address pitch of inner for-loop, vise versa. However, when reshape join the game, the stride is not a fixed value, for example:

    Matrix3i m;
    m << 1, 2, 3,
         4, 5, 6,
         7, 8, 9;
    
    m.block(0, 0, 2, 2);               // 1, 2      inner:1, outer:3
                                       // 4, 5
    
    m.block(0, 0, 2, 2).reshape(4, 1); // 1         inner:1 or 2
                                       // 4
                                       // 2
                                       // 5
    

    Currently I have no idea how to solve this problem, so I will turn off the direct access for reshape expression. Maybe a specialized version for Matrix class?

  12. Jitse Niesen

    Ad 1: As I see it, this is a design issue with the core of Eigen, and cannot be fixed properly in reshape.

    Ad 2: In your example direct access is not possible. Indeed, I think we need a specialized version for Matrix or something like that.

    I could not get the unit test to compile. The first error is in line 21 of reshape.cpp. If I remove the .eval() call (which I do not understand anyway), everything goes fine except for the chain of reshapes at the end where the INVALID_STORAGE_ORDER_FOR_THIS_VECTOR_EXPRESSION is triggered.

  13. 蕭刻庭 author

    Sorry about the stupid test case build problem.

    I was trying to disable the DirectAccessBit and accidentally enable the IsRowMajorBit. It is fixed.

    I will try write the specialization of Reshape<Matrix> to gain the DirectlyAccess optimization.

    Thanks for reviewing my code.

  14. Jitse Niesen

    I realized this morning that, instead of writing a specialization for Reshape<Matrix>, it is perhaps also possible (and more general) to look at the linear access flag. I think that if that flag is set and the direct access flag, then there are no holes between the columns and the inner stride in the reshape'd expression will be the same.

    Your example made me realize one more issue: we cannot always use packet access in the reshape'd expression, because columns can be broken up and joined in arbitrary ways. For example, this does not work if vectorization is enabled (with 32-bits gcc, you have to pass the -msse2 flag to see this):

      MatrixXd A(5,5);
      for (int i=0; i<A.rows(); ++i) for (int j=0; j<A.cols(); ++j) A(i,j) = 10*i+j;
      cout << "A = \n" << A << "\n\n";
      cout << "A.block(0,0,3,2) = \n" << A.block(0,0,3,2) << "\n\n";
      MatrixXd res = A.block(0,0,3,2).reshape(2,3);
      cout << "res = \n" << res << "\n";
    

    I think PacketAccess is safe if the underlying expression has both PacketAccess and LinearAccess set. It is probably also safe in some fixed-size situations, but I'm happy to ignore that for now.