Homogeneous spaces: Stiefel and Grassmann manifolds#156
Draft
vabor112 wants to merge 67 commits into
Draft
Conversation
vabor112
commented
May 29, 2025
Member
Author
vabor112
left a comment
vabor112
left a comment
There was a problem hiding this comment.
Thanks for the PR draft. Here's some things to do:
- Make sure to put type annotations wherever possible, as well as docstrings describing all parameters and return values, in the same style as in other modules.
- Make sure checks are successful (it's important that you do this as it might be nontrivial to fix some things, e.g. when some backends work and some don't because of the constructions that you use).
- Make sure there are no errors in the notebooks.
- Add tests: for instance, check averaging converges to the analytic spherical functions for Grassmannian, check that when Stiefel/Grassmannian reduce to simple special cases, such as SO or sphere, they align with the spaces already implemented in the library.
Also there are a few comments attached to the specific places in code.
| kernel.num_levels, | ||
| MaternGeometricKernel._DEFAULT_NUM_RANDOM_PHASES, | ||
| ) | ||
| if isinstance(kernel.space, CompactHomogeneousSpace): |
Member
Author
There was a problem hiding this comment.
Is this separateif only to set different default_num?
| @overload | ||
| def feature_map_from_kernel(kernel: MaternKarhunenLoeveKernel): | ||
| if isinstance(kernel.space, CompactMatrixLieGroup): | ||
| if isinstance(kernel.space, CompactMatrixLieGroup) or isinstance(kernel.space, Grassmannian): |
Member
Author
There was a problem hiding this comment.
For some reason, in feature_map_from_space below, the behavior is different
| @overload | ||
| def default_num(space: DiscreteSpectrumSpace) -> int: | ||
| if isinstance(space, CompactMatrixLieGroup): | ||
| if isinstance(space, CompactMatrixLieGroup) or isinstance(space, CompactHomogeneousSpace): |
Member
Author
There was a problem hiding this comment.
I'd suggest to introduce a separate _DEFAULT_NUM_LEVELS_COMPACT_HOMOGENEOUS, it might be equal to _DEFAULT_NUM_LEVELS_LIE_GROUP
| X2_inv = self.inverse(X2) | ||
| X_ = B.tile(X[..., None, :, :], 1, X2_inv.shape[0], 1, 1) # (N, N2, n, n) | ||
| X2_inv_ = B.tile(X2_inv[None, ..., :, :], X.shape[0], 1, 1, 1) # (N, N2, n, n) | ||
| if inverse_X: |
Member
Author
There was a problem hiding this comment.
Why did you need to change Lie groups stuff? The change is not backend-independent btw
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Finalizing the support of
Stationary Kernels and Gaussian Processes on Lie Groups and their Homogeneous Spaces I: the compact case(https://arxiv.org/abs/2208.14960).