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

Symplectic Grassmann #700

Merged
merged 70 commits into from
Jan 25, 2024
Merged

Symplectic Grassmann #700

merged 70 commits into from
Jan 25, 2024

Conversation

kellertuer
Copy link
Member

@kellertuer kellertuer commented Jan 3, 2024

this PR is a start to implement the (real) Symplectic Grassmann, which one obtains from Symplectic Stiefel the same way one obtains Grassmann from Stiefel.
It will probably also revise a few symplectic Stiefel functions.

  • Introduce Hamiltonian, they allow for H^+ to be nicely defined for the symplectic inverse
  • We should introduce HamiltonianMatrices? Maybe not 100% necessary but nice for completeness
  • Define SymplecticGrassmann
  • implement checks
  • inner (after (4.10) / Lemma 4.8, BZ21)
  • the test/utils.jl functions complain since Julia 1.10 to be multiply included. We should at least move those, if not also the constants to the Manufolds.jl/Test.jl extension?
  • unify notation concerning J_2n and Q_2n for the symplectic element (and refer to SymplecticMatrix).
  • unify/simplify notation to refer more often to symplectic_inverse when writing p^+ instead of repeating the formula.
  • implement the geodesic
  • implement the Cayley retraction and its inverse
  • refactor rand_hamiltonian to be rand(Hamiltonian(2n))
  • implement the gradient conversion (after Lemma 4.8, BZ21)
  • (maybe) check for Symplectic and SymplecticStiefel, so they get rand! methods as well. They currently just have rand methods
  • read docs carefully again
  • check test coverage

Both symplectic Stiefel and Grassmann stem from this paper https://arxiv.org/abs/2108.12447

Copy link

codecov bot commented Jan 3, 2024

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (64ae41a) 99.54% compared to head (9500bd5) 94.87%.

Files Patch % Lines
src/manifolds/SymplecticGrassmannStiefel.jl 90.47% 4 Missing ⚠️
src/manifolds/Hamiltonian.jl 95.31% 3 Missing ⚠️
src/manifolds/Symplectic.jl 97.90% 3 Missing ⚠️
src/manifolds/SymplecticGrassmann.jl 92.85% 1 Missing ⚠️
src/manifolds/SymplecticGrassmannProjector.jl 96.55% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #700      +/-   ##
==========================================
- Coverage   99.54%   94.87%   -4.67%     
==========================================
  Files         108      112       +4     
  Lines       10715    10871     +156     
==========================================
- Hits        10666    10314     -352     
- Misses         49      557     +508     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kellertuer kellertuer added new manifold This issue proposes to implement a new manifold preview docs Add this label if you want to see a PR-preview of the documentation labels Jan 5, 2024
NEWS.md Outdated Show resolved Hide resolved
@kellertuer kellertuer marked this pull request as ready for review January 19, 2024 15:41
@kellertuer
Copy link
Member Author

With the last few methods now I would consider this feature-complete (just not yet test&docs complete), so I moved it from draft to ready (but did not yet set the label).

Copy link
Member

@mateuszbaran mateuszbaran left a comment

Choose a reason for hiding this comment

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

I haven't read the actual new stuff but here are some small technical comments.

NEWS.md Outdated Show resolved Hide resolved
docs/src/features/atlases.md Outdated Show resolved Hide resolved
docs/src/manifolds/fiber_bundle.md Outdated Show resolved Hide resolved
src/manifolds/GeneralizedGrassmann.jl Show resolved Hide resolved
src/manifolds/QuotientManifold.jl Outdated Show resolved Hide resolved
test/ambiguities.jl Outdated Show resolved Hide resolved
test/groups/group_utils.jl Show resolved Hide resolved
@kellertuer kellertuer added the Ready-for-Review A label for pull requests that are feature-ready label Jan 24, 2024
@kellertuer
Copy link
Member Author

This now covers all I wanted to be included. I already bumped the version, but I would also like to include the Multinomial PR (#702) in this version I think, since that one is basically finished as well.

@mateuszbaran
Copy link
Member

It looks like tolerance in one test for SymplecticStiefel is too low.

@kellertuer
Copy link
Member Author

I am already running this locally. I am a bit surprised by that since I did not change that.

Copy link
Member

@mateuszbaran mateuszbaran left a comment

Choose a reason for hiding this comment

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

A few comments.

src/manifolds/Hamiltonian.jl Outdated Show resolved Hide resolved
src/manifolds/Hamiltonian.jl Outdated Show resolved Hide resolved
src/manifolds/SymplecticGrassmann.jl Outdated Show resolved Hide resolved
src/manifolds/SymplecticGrassmann.jl Outdated Show resolved Hide resolved
src/manifolds/SymplecticGrassmannStiefel.jl Outdated Show resolved Hide resolved
src/manifolds/SymplecticGrassmannStiefel.jl Outdated Show resolved Hide resolved
src/manifolds/Hamiltonian.jl Outdated Show resolved Hide resolved
src/manifolds/Hamiltonian.jl Outdated Show resolved Hide resolved
src/manifolds/Hamiltonian.jl Outdated Show resolved Hide resolved
src/manifolds/Hamiltonian.jl Outdated Show resolved Hide resolved
src/manifolds/SymplecticGrassmann.jl Show resolved Hide resolved
src/manifolds/SymplecticGrassmannStiefel.jl Outdated Show resolved Hide resolved
src/manifolds/SymplecticGrassmannStiefel.jl Outdated Show resolved Hide resolved
src/manifolds/SymplecticGrassmannStiefel.jl Show resolved Hide resolved
Copy link
Member

@mateuszbaran mateuszbaran left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@kellertuer kellertuer merged commit 95e90c8 into master Jan 25, 2024
24 of 26 checks passed
@kellertuer kellertuer mentioned this pull request Jan 25, 2024
4 tasks
@kellertuer kellertuer deleted the kellertuer/more-grassmann branch May 4, 2024 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new manifold This issue proposes to implement a new manifold preview docs Add this label if you want to see a PR-preview of the documentation Ready-for-Review A label for pull requests that are feature-ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants