Refactor invariant metrics #1948
Conversation
luisfpereira
force-pushed
the
invariant-metrics
branch
from
6262cc8
to
65e117d
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1948 +/- ##
==========================================
+ Coverage 91.24% 91.57% +0.33%
==========================================
Files 146 151 +5
Lines 13515 13706 +191
==========================================
+ Hits 12331 12550 +219
+ Misses 1184 1156 -28
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
|
||
| self.n = n | ||
| self.epsilon = epsilon | ||
|
|
||
| self.skew = SkewSymmetricMatrices(self.n, equip=False) | ||
|
|
||
| def equip_with_metric(self, Metric=None, **metric_kwargs): |
There was a problem hiding this comment.
Missing docstring and parameter descriptions.
|
|
||
| return self | ||
|
|
||
| def equip_with_metric(self, Metric=None, **metric_kwargs): |
There was a problem hiding this comment.
Missing parameter descriptions
luisfpereira
force-pushed
the
invariant-metrics
branch
from
65e117d
to
ba660ae
Compare
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
luisfpereira
force-pushed
the
invariant-metrics
branch
3 times, most recently
from
c295954
to
a7f8724
Compare
There was a problem hiding this comment.
Awesome work (again!).
High-level:
See question about equip with biinvariant metric, which might not be correct, because some groups simply don't have any biinvariant metric.
See comment on Euclidean as a lie algebra: we can't call it this way bcs it doesn't have a lie bracket?
Otherwise, minor docstrings picknits, and then good to merge!
| ) | ||
| return self | ||
|
|
||
| def _equip_lie_algebra_for_biinvariant_metric(self): |
There was a problem hiding this comment.
Some Lie groups do not admit any bi-invariant metric. Thus, I'm not sure what this function is really doing?
Only compact group, abelian groups, and their products, admit bi-invariant metrics. For example, SE(n) never admits any bi-invariant metric.
|
|
||
| return self | ||
|
|
||
| def equip_with_metric(self, Metric=None, **metric_kwargs): |
| super().__init__(dim=dim, shape=(dim,), lie_algebra=self, equip=equip) | ||
|
|
||
| super().__init__( | ||
| dim=dim, shape=(dim,), lie_algebra=Euclidean(dim, equip=False), equip=equip |
There was a problem hiding this comment.
from a design perspective, lie_algebra = Euclidean feels a bit weird, because Euclidean doesn't have a lie bracket, thus how could it be a lie algebra?
|
|
||
| self.n = n | ||
| self.epsilon = epsilon | ||
|
|
||
| self.skew = SkewSymmetricMatrices(self.n, equip=False) | ||
|
|
||
| def equip_with_metric(self, Metric=None, **metric_kwargs): |
…ment inner_product_at_identity
luisfpereira
force-pushed
the
invariant-metrics
branch
from
a7f8724
to
f3d8e35
Compare
This PR builds on top of #1946 and #1947 to refactor invariant metrics. The main idea is to remove
inner_product_at_identityand uselie_algebra.metric.inner_product. This implieslie_algebrais equipped with an appropriate metric.Besides simplifying the code and making it easier to test, this PR opens the door to use nondiagonal metric matrices at identity (will partially solve e.g. #1884). This is already implemented and tested, but algorithms need to be made more robust.
Still need to refactor
_InvariantMetricVector, but will be left for another PR.NB: merging this PR before #1946 and #1947 also merges them.