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

Add Complex Grassmann manifold #112

Merged
merged 6 commits into from
Jul 27, 2020

Conversation

antoinecollas
Copy link
Contributor

Hello,
I have implemented the complex Grassmann manifold following the implementation of Manopt. I also implemented the corresponding unit tests.

@coveralls
Copy link

coveralls commented Apr 22, 2020

Coverage Status

Coverage increased (+0.4%) to 86.26% when pulling 2c57fa0 on antoinecollas:ComplexGrassmann into d16b088 on pymanopt:master.

class ComplexGrassmann(Manifold):
"""
Factory class for the Grassmann manifold. This is the manifold of p-
dimensional subspaces of n dimensional real vector space. Initiation
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be complex vector space, no?

np_testing.assert_almost_equal(self.man.norm(X, self.man.log(X, Y)),
self.man.dist(X, Y))

# def test_ehess2rhess(self):
Copy link
Contributor

@sylvchev sylvchev May 22, 2020

Choose a reason for hiding this comment

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

You could remove this commented code if you don't plan to use it

Copy link
Contributor

@sylvchev sylvchev left a comment

Choose a reason for hiding this comment

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

Thank you for this nice PR!
Could you also consider to add some example for demonstration purpose?

@antoinecollas
Copy link
Contributor Author

I corrected what was stated in the review.

I have used this manifold for my personal needs but I have no plans to add an example at this time.

@sweichwald
Copy link
Member

Thanks for your contribution @antoinecollas and for looking into it @sylvchev 👍

From my side, this is ready to be merged; same remarks as for #113 apply.

sylvchev
sylvchev previously approved these changes Jul 15, 2020
sweichwald
sweichwald previously approved these changes Jul 15, 2020
@sweichwald sweichwald dismissed stale reviews from sylvchev and themself via 502c0df July 27, 2020 09:45
@sweichwald sweichwald merged commit 3bee695 into pymanopt:master Jul 27, 2020
@antoinecollas antoinecollas deleted the ComplexGrassmann branch January 24, 2022 08:25
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.

4 participants