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

Implements support for Complex #5

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

spinicist
Copy link

Finished a first pass at complex support.

@spinicist spinicist mentioned this pull request May 24, 2022
@LTLA
Copy link
Owner

LTLA commented May 25, 2022

Looks great. Only a few minor comments:

  • No need for Type in all the template parameter names, should make it a bit less verbose. I think the PascalCasing should make it obvious enough.
  • I wonder about the use of RowsAtCompileTime in the Vector typedef's. I don't think there's any guarantee that the desired vector sizes will be equal to the rows of the Matrix class if someone were to use Matrix2/3d instances. For example, various internal vectors in irlba::Irlba::run should have length equal to work, which doesn't have an obvious relation to any compile-time dimension of Matrix. It may be better just to always use Dynamic in the typedefs, to avoid the implication that we support compile-time lengths.

Some @tparam documentation needs to be added but I can do that later.

Edit: Working on it now.

Merged changes from the master branch.
@LTLA
Copy link
Owner

LTLA commented May 26, 2022

Alright, after a bit of contemplation, I did some surgery to change the templating strategy.

The problem with initial design is that the parametrization of the Eigen matrix type was done at the class level and thus divorced from the input matrix used in run(). This meant that the caller needs to make sure that an appropriate (or the same) type is used both times; in the construction of the Irlba object, and in the call to run().

My edit shifts the templating so that it is all handled by run(), so there's only one place to specify the type. In fact, the Eigen matrix type is now derived from the scalar type used in the input matrix, so it doesn't even need to be specified. This is safer as it avoids accidents from people providing a complex matrix and requesting an Eigen::MatrixXd output.

I also added the documentation, switched to Dynamic, and truncated the template parameter names as I mentioned above.

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.

2 participants