8000 Update discrete curves elastic metric for ratio equal to 1 by luisfpereira · Pull Request #1992 · geomstats/geomstats · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Update discrete curves elastic metric for ratio equal to 1 #1992

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

Merged
merged 2 commits into from
Apr 12, 2024

Conversation

luisfpereira
Copy link
Collaborator
@luisfpereira luisfpereira commented Apr 11, 2024

This PR updates discrete curves elastic metric to handle the case where a / 2b = 1, for any ambient dimension, by using theorem 2.1 of (Bauer, 2022). It also uses the theorem 2.1 equation to improve testing (validates our diffeomorphism-based approach).

Notice how easy this implementation is, suggesting our abstractions are not that bad! Here we use: Diffeo, PullbackDiffeoMetric, ScalarProductMetric, NFoldManifold, NFoldMetric. Besides the specific implementation for the diffeos and the metric in the image space, we are simply reusing code without any specialization.

Next steps: extend implementation for ratios other than 1. Only need to implement a different metric for the image space.

@luisfpereira luisfpereira force-pushed the curves-elastic-metrics branch from 1942e9a to 3ed573f Compare April 11, 2024 10:14
Copy link
Collaborator
@ninamiolane ninamiolane left a comment

Choose a reason for hiding this comment

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

LOVE IT! Indeed, the abstractions work beautifully.

I just have one question, otherwise good to merge. Thank you!

from geomstats.vectorization import get_batch_shape
8000

def elastic_distance(space, point_a, point_b, a=1.0, b=0.5):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this overwrite elastic_metric.dist ? I guess this depends on the answer to the question: which one is faster 1) our current implementation of dist that uses our classes and the diffeo abstraction, or 2) this function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Spot on!

A quick test has shown elastic_distance to be faster (though both are fast).
Still, I have a refactoring in mind for NFoldMetric that is likely to put both on pair or even make the current one faster (it will take advantage of the fact most operations are vectorized for multiple batch dimensions).

I suggest to merge as is, and check this again after the refactoring I'm suggesting for the NFoldMetric.

(We can actually make elastic_distance even faster by computing the length directly from the velocity (since we use the point, we need to do finite differences again inside).)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We'll check this again later @ninamiolane. I'm merging now as it is useful.

@luisfpereira luisfpereira merged commit a5bce8c into geomstats:main Apr 12, 2024
@luisfpereira luisfpereira deleted the curves-elastic-metrics branch April 12, 2024 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0