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

add geodesic to pullbackdiffeometric #1877

Conversation

Abdellaoui-Souhail
Copy link
Contributor

Checklist

  • My pull request has a clear and explanatory title.
  • If necessary, my code is vectorized.
  • I added appropriate unit tests.
  • I made sure the code passes all unit tests. (refer to comment below)
  • My PR follows PEP8 guidelines. (refer to comment below)
  • My PR follows geomstats coding style and API.
  • My code is properly documented and I made sure the documentation renders properly. (Link)
  • I linked to issues and PRs that are relevant to this PR.

Description

Issue

Additional context

@codecov
Copy link

codecov bot commented Jul 25, 2023

Codecov Report

Merging #1877 (82e62cf) into master (6386ade) will increase coverage by 8.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1877      +/-   ##
==========================================
+ Coverage   82.59%   90.72%   +8.13%     
==========================================
  Files         136      140       +4     
  Lines       13577    13071     -506     
==========================================
+ Hits        11213    11857     +644     
+ Misses       2364     1214    -1150     
Flag Coverage Δ
autograd 87.01% <100.00%> (?)
numpy 87.35% <85.72%> (?)
pytorch ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
geomstats/geometry/pullback_metric.py 92.07% <100.00%> (+6.35%) ⬆️

... and 66 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more


Parameters
----------
point_a : array-like, shape=[..., *shape]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Correct the names of parameters

Copy link
Collaborator

Choose a reason for hiding this comment

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

point_a --> initial_point, etc

Returns
-------
geodesic : callable
Geodesic between point_a and point_b.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Idem: it's either between initial point and end point, or with initial point and initial tangent vec.

image_point_a, image_point_b, image_initial_tangent_vec
)

def geod_function(t):
Copy link
Collaborator

Choose a reason for hiding this comment

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

rename to path for consistency with the code base.

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.

Thanks!

Some minor comments on the code style for the geodesic function. Next, remove the tests and add the inheritance from RiemannianMetricTestCase. Hopefully most tests will pass!


Parameters
----------
point_a : array-like, shape=[..., *shape]
Copy link
Collaborator

Choose a reason for hiding this comment

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

point_a --> initial_point, etc

geodesic : callable
Geodesic between point_a and point_b.
"""
image_point_a = self.diffeomorphism(initial_point)
Copy link
Collaborator

Choose a reason for hiding this comment

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

fix variable names for consistency.
for example: image_point_a --> image_initial_point

@@ -1696,6 +1697,41 @@ def test_matrix_innerproduct_and_embedded_innerproduct_coincide(
"""
# Not yet implemented due to need for local basis implementation

def test_dist_vectorization(self, space, n_samples):
Copy link
Collaborator

Choose a reason for hiding this comment

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

As discussed, these tests are not needed as long as you make PullbackDiffeoMetricTestCase inherit from RiemannianMetricTestCase.

--> Do this and let's see what happens, typically which tests we might need to skip?

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.

Great, thanks!

@ninamiolane ninamiolane merged commit 2eeee17 into geomstats:master Jul 27, 2023
14 of 15 checks passed
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.

None yet

2 participants