General elastic metric #1078
General elastic metric #1078
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1078 +/- ##
==========================================
- Coverage 94.50% 94.08% -0.41%
==========================================
Files 89 81 -8
Lines 8895 7901 -994
==========================================
- Hits 8405 7433 -972
+ Misses 490 468 -22
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Excellent!! The code is very clean, thank you 🎉
There are:
- 1 deepsource error
- 1 comment about whether we really need a new ElasticCurve class
- minor comments here and there
- errors in pytorch and tf: if you can solve them, that would be amazing! otherwise we can use the decorators.
Ready to merge soon IMO! Congrats!
|
|
||
| return norms, args | ||
|
|
||
| def polar_to_cartesian(self, norms, args): |
There was a problem hiding this comment.
You can remove self and make it a @staticmethod (cf remaining DeepSource issue)
| @@ -842,3 +842,340 @@ def g_criterion(srv, srv_norms): | |||
| nb_iter += 1 | |||
|
|
|||
| return proj | |||
|
|
|||
|
|
|||
| class ElasticMetric(RiemannianMetric): | |||
|
|
||
| def f_transform(self, curve): | ||
| """Compute the F_transform of a curve. | ||
|
|
There was a problem hiding this comment.
Could you add the formula using :math: in the docstring here? (see other docstrings in geomstats where this has been done).
| Returns | ||
| ------- | ||
| f : array-like, shape=[..., n_sampling_points - 1, ambient_dim] | ||
| F_transform of the curve.. |
There was a problem hiding this comment.
Nit: only one period after curve.
| def f_transform_inverse(self, f, starting_point): | ||
| """Compute the inverse F_transform of a transformed curve. | ||
|
|
||
| Only works if a/2b <= 1. |
There was a problem hiding this comment.
Maybe add a test in this function, that will raise an error if a/2b>1?
|
|
||
| def f_transform_inverse(self, f, starting_point): | ||
| """Compute the inverse F_transform of a transformed curve. | ||
|
|
There was a problem hiding this comment.
If we have an analytic form for the inverse, could you add it in :math: in the docstring too? thank you! 🙌
| return distance | ||
|
|
||
|
|
||
| class ElasticCurves(Manifold): |
There was a problem hiding this comment.
Why do we need a new class here? How about: adding the elastic metric as an attribute of the existing classes of DiscreteCurves and/or ClosedDiscreteCurves (I don't remember if it would work in the closed curves case?).
There was a problem hiding this comment.
Indeed, I just thought DiscreteCurves was reserved for the srv metric.
There was a problem hiding this comment.
Actually I'm not sure of how I should add the elastic metric as an attribute of DiscreteCurves since it only works for planar discrete curves (i.e. ambient_manifold = Euclidean(dim=2).
There was a problem hiding this comment.
You could add if dim == 2: self.elastic_metric = ...?
| plane curves", arXiv:1803.10894 [math.DG], 29 Mar 2018. | ||
| """ | ||
|
|
||
| def __init__(self, a, b): |
There was a problem hiding this comment.
Should we call the SRVMetric if a = 1 and b = 1? Or at least explain in the docstring how the two metrics relate? @Florent-Michel
There was a problem hiding this comment.
SRVMetric and ElasticMetric(1, 0.5) are indeed almost the same (up to a factor which depends on the number of sampling points because I made different normalization choices)
There was a problem hiding this comment.
I think that for coherence it is important that the results of SRVMetric and ElasticMetric(1, 0.5) are the same. If it's just a normalization choice, can it be easily changed in SRVMetric ? And maybe add a test to check that both results are the same.
|
@Florent-Michel any update on this? |
No description provided.