HermitianMatrix

#467 Open
Repository
tellenbach
Branch
HermitianMatrix
Repository
eigen
Branch
default

Bitbucket cannot automatically merge this request.

The commits that make up this pull request have been removed.

Bitbucket cannot automatically merge this request due to conflicts.

Review the conflicts on the Overview tab. You can then either decline the request or merge it manually on your local system using the following commands:

hg update default
hg pull -r HermitianMatrix https://bitbucket.org/tellenbach/eigen
hg merge HermitianMatrix
hg commit -m 'Merged in tellenbach/eigen/HermitianMatrix (pull request #467)'
Author
  1. David Tellenbach
Reviewers
Description

This pull request is part of the Google Summer of Code 2018 project Faster Matrix Algebra for ATLAS (https://summerofcode.withgoogle.com/projects/#5293950017994752). It contains an implementation for using hermitian matrices stored in Rectangular Full Packed Format (RFPF). See http://www.netlib.org/lapack/lawnspdf/lawn199.pdf for further information about this storage scheme.

What is implemented?

  • A class template HermitianBase that is a base class for hermitian expressions but contains no storage
  • A class template HermitianMatrix that represents a HermitianMatrix with storage in RFPF
  • Core evaluators for HermitianMatrix
  • Product evaluators for building the following products:

    hermitian = hermitian * hermitian dense = hermitian * hermitian dense = hermitian * dense dense = dense * hermitian

  • IO support for instances of HermitianMatrix

What should be improved?

  • More generic implementation of CwiseBinaryOp for HermitianMatrix (currently only the special cases of + and - is implemented)
  • Better support for aliasing
  • Adding tests and examples

Even though my Google Summer of Code 2018 participation ends with this pull request, I’m heavily interested in continuing contributing to Eigen and make my implementation ready to merge. Therefore I would be very happy about some feedback and ideas on how to improve the current implementation.

Comments (7)

  1. Stewart Martin-Haugh

    Hi @ggael , @chtz ,

    We would welcome feedback on this PR.

    Best regards,

    Stewart (David’s GSOC mentor)

    1. Christoph Hertzberg

      I think I said this on the mailing list already: I would have preferred if this supported Triangular matrices, which then could be used using SelfAdjointView to represent Hermetian matrices.

      Are we sure that we only want to support RFPF, or maybe simple lexicographical order as well (configurable via a flag)? Especially, when mapping external data both may be useful.

      Also, perhaps this should first be made an unsupported module -- until the API is more or less complete and agreed upon.

      1. Stewart Martin-Haugh

        Hi Christoph,

        Many thanks for the review comments!

        From the discussion on the mailing list, I think @martond was keen for Eigen to follow LAPACK and only support RFPF.

        Indeed making this an unsupported module initially seems like a good idea.

        @tellenbach should also comment, of course.

        Cheers,

        Stewart

        1. David Tellenbach author

          Thanks for the comments.

          I would have preferred if this supported Triangular matrices, which then could be used using SelfAdjointView to represent Hermitian matrices.

          I guess I have misunderstood this during our initial discussion on the mailing list. However this might be not too hard to add since the underlying storage won't be affected. While running some benchmarks I saw that using SelfAdjointView is not really faster than using dense multiplication, at least for matrices that are not really big.

          Are we sure that we only want to support RFPF, or maybe simple lexicographical order as well (configurable via a flag)? Especially, when mapping external data both may be useful.

          This is something we could definitely keep in mind. I think additional storage models could be implemented on top of HermitianBase . During our initial discussion on the mailing list it was indeed pointed out that supporting packed storage might be not worth.

          Also, perhaps this should first be made an unsupported module -- until the API is more or less complete and agreed upon.

          As @StewMH already mentioned we are definitely fine with making this an unsupported module. Should I move modification of already existing files like IO.h or MatrixBase.h into other new files for this purpose?

          Thanks for the code comments, I will try to answer them / improve the critical parts soon.

          Thanks,

          David

          1. Christoph Hertzberg

            Sorry for not answering to this one yet.

            Products with SelfAdjointView could only be faster if the full matrix would overflow the cache, but the self-adjoint does not. If you don't enable multi-threading (which could easily be inserted into SelfAdjoint products as well), I don't think you are faster either (I did not benchmark that). The advantage would be to have a consistent API and, getting lots of functionality "for free".

            Non-RFP-formats don't need to be addressed yet, I guess this can be added later, by adding flags into the Storage template parameter. I think in the long run, this is worth it, as soon as we have Map<HermitianMatrix> support.

            For separating this into a different module: Yes, as long as you don't modify existing types you can move everything into separate files -- specializing existing template classes is fine as well (i.e., also possible in separate files).

            1. David Tellenbach author

              Thanks for your response.

              I see the advantages of your approach and still believe it might be not too hard to modify the existing implementation in that way. However the current implementation is faster in multiplying large matrices (see my posts to the mailing list) and therefore also has an advantage. I could even imagine using the class HermitianMatrix as a wrapper for SelfAdjointView<TriangularMatrix, ...> to provide an easy to use interface or using it as a typedef.

              I am totally willing to modify the implementation but think this is a broader design decision that should maybe be addressed to the mailing list again?

              Considering non-RFP-formats I agree with you and will keep it in mind.

              I think I haven't modified the existing code base (expect template specializations) and moving everything into separate files should be no problem. Is modifying ForwardDeclarations.h okay or should #include <...> be used?

              1. Christoph Hertzberg

                Hi!

                Having a SelfAdjointView around a RFPF-Triangular matrix should in the end be equivalent to your current implementation. Feel free to ask about any design decision on the mailing list, though. And if this will be added as an unsupported module, I am quite liberal with any (in my opinion) sub-optimal API. What really is necessary before this can be merged is a set of unit-tests, however.

                Adding things to ForwardDeclarions.h generally is okay. I'm not sure, if it really is necessary in your case (usually, you only need it, if you have some kind of circular references).