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

Refactor/clean module of discrete curves #1183

Open
2 of 5 tasks
ninamiolane opened this issue Oct 22, 2021 · 12 comments
Open
2 of 5 tasks

Refactor/clean module of discrete curves #1183

ninamiolane opened this issue Oct 22, 2021 · 12 comments

Comments

@ninamiolane
Copy link
Collaborator

ninamiolane commented Oct 22, 2021

There are a few tasks that remain to be done to clean-up or refactor the module of discrete curves:

  • Address the comments from @Florent-Michel 's PR General elastic metric #1078 to clean the code:
    • In particular, we need to have consistency between SRVMetric and ElasticMetric(1, 0.5), which is not the case at the moment.
  • Address the problem that "A. Srivastava, E. Klassen, S. H. Joshi and I. H. Jermyn, in "Shape Analysis of Elastic Curves in Euclidean Spaces," quotient the space of curves by the action of the translation, but our implementation does not (while we are citing their paper).
    • We could made the quotient by the translation as an option.
  • Use the structure of quotient metric to organize the code.
@ninamiolane
Copy link
Collaborator Author

The class QuotientSRVMetric should not inherit from SRVMetric.

Indeed, with this inheritance, whenever a method is not found in QuotientSRVMetric, it is taken from its parent class SRVMetric - which is not correct.

For example, QuotientSRVMetric does not have a log implemented, and should not take the log from SRVMetric.

@ninamiolane
Copy link
Collaborator Author

Same for ClosedSRVMetric. Using QuotientSRVMetric or ClosedSRVMetric within FrechetMean will run, but the estimated mean will not be the frechetmean corresponding to these metrics --- because of this inheritance.

@ninamiolane
Copy link
Collaborator Author

@alebrigant what do you think about this paper to compute the distance (only the distance) in the quotient SRV space:
https://arxiv.org/pdf/1501.00577.pdf: PRECISE MATCHING OF PL CURVES IN RN IN THE SQUAREROOT VELOCITY FRAMEWORK (Sahini, Robinson, Klassen - 2015)

@ninamiolane
Copy link
Collaborator Author

ninamiolane commented May 4, 2022

ElasticMetric does not have log, exp and geodesics, which should be added. Florent-Michel should have code for the geodesic.

@ninamiolane
Copy link
Collaborator Author

'DiscreteCurves' object has no attribute 'random_uniform'

@ninamiolane
Copy link
Collaborator Author

DiscreteCurves should also inherit from Landmarks and have a fixed number of sampling points. This would be more consistent with the code base, and avoid math.inf in the dimensions of the manifold.

@ninamiolane
Copy link
Collaborator Author

ninamiolane commented Jun 22, 2022

  • SRVMetric should inherit from ElasticCurve
  • srv_transform and srv_transform inverse should overwrite f_transform and f_transform_inverse

@ninamiolane
Copy link
Collaborator Author

ninamiolane commented Jun 22, 2022

  • f_transform_inverse should be unit-tested

@ninamiolane
Copy link
Collaborator Author

ninamiolane commented Aug 11, 2022

  • the API of the discrete curves module should be cleaned up: so far, it alternates between calling curves curve_a, curve_b and point_a, point_b where the latter comes from the consistency with the parent classes which refer to elements on manifolds as "points".

see issue #1484

@ninamiolane
Copy link
Collaborator Author

PR #1632 addresses the API issue.

@ninamiolane
Copy link
Collaborator Author

Copy-pasting here a comment from PR #1632 @alebrigant

Re name: SRVShapeBundle:

I wonder if we should put the name of the group inducing the quotient in the name of the class.

We could have a SRVShapeBundle only by quotienting the action of rotations.

Actually, in these shape manifolds, we will have a series of quotient, for each additional group applied.

Thus: SRVRotationBundle, SRVParamBundle?

@alebrigant
Copy link
Collaborator

I agree we need to rethink the way we handle different quotients. Maybe the best way, from a user-perspective, would be to have a class SRVShapeBundle with boolean attributes quotient_param, quotient_rotations, quotient_translations that can be set to True or False so that the user defines what type of "shape" she/he wants to use ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants