Sasaki metric #1568
Sasaki metric #1568
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1568 +/- ##
==========================================
+ Coverage 88.52% 90.89% +2.37%
==========================================
Files 104 110 +6
Lines 10263 10594 +331
==========================================
+ Hits 9084 9628 +544
+ Misses 1179 966 -213
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Very nice, thank you! May I let you address the lint errors in the Github workflows:
I will let @LPereira95 comment on the tests, and use of pytorch/tensorflow backends. |
|
I fixed the linting/deepsource issues (Originally I kept the --ignoreD flag for flake8; that introduced errors I did not notice in advance). However, it is a bit tricky that DeepSource somehow spots similar but different problems as flake8 (i.e. ). Can DeepSource somehow be run before initiating a PR? The tests for the tf and torch backends are failing because right now the implementation only supports np and autograd. However, I used flags to enable testing for np/autograd only and I have no idea why the tf and torch test are still carried out. |
There was a problem hiding this comment.
Thanks for the very interesting contribution @ambellan!
As you can see, I have several remarks to make. Most of them are related with vectorization. Addressing my suggestions will increase the code speed and probably make the code work with all the backends.
I'm looking forward to your feedback on this.
geomstats/geometry/sasaki_metric.py
Outdated
| def __init__(self, metric: RiemannianMetric, n_s=3): | ||
| self.metric = metric # Riemannian metric of underlying space | ||
| self.n_s = n_s # Number of discretization steps | ||
| shape = (2, gs.prod(metric.shape)) |
There was a problem hiding this comment.
Should shape be (2, gs.prod(metric.shape) or (2, *metric.shape) or (2*metric.shape[0], *metric.shape[1:])?
I think (2, *metric.shape) is the right one.
There was a problem hiding this comment.
Well, this would introduce a difference if metric.shape consists of more than one element. I am not shure whether this should be changed as it my introduce problems. Ok, I just checked back with @vontycowicz and he confirmed that (2, *metric.shape) did not work together with the Frechet-mean in Kendall shape space. For the moment we would like to keep it as is, unrolling metric.shape. This may be changed later on.
Replacing (2, gs.prod(metric.shape) by (2, gs.prod(gs.array(metric.shape)) already allows to successfully pass the unit tests employing the pytorch backend.
There was a problem hiding this comment.
Do you know why the unrolling hasn't worked with Frechet mean in Kendal shape space? Can you share a small reproducible example of the error?
The goal of unrolling would be that instead of reshaping to (-1, 2) + self.metric.shape later in the code, we could do (-1,) + self.shape. It would be more robust as we could control reshaping shape from one place only.
There was a problem hiding this comment.
@LPereira95, When taking (2, *metric.shape) instead of (2, gs.prod(metric.shape) problems in the FrechetMean class at different locations, here with two examples:
-
In the constructor:
if point_type is None: self.point_type = metric.default_point_type error.check_parameter_accepted_values( self.point_type, "point_type", ["vector", "matrix"] )
We would have something like 3-tensor as point_type and that is not handled. -
In the _default_gradient_descent
if point_type == "vector": points = gs.to_ndarray(points, to_ndim=2) einsum_str = "n,nj->j" else: points = gs.to_ndarray(points, to_ndim=3) einsum_str = "n,nij->ij"
There is no handling for point_types different from vector and matrix.
Apart form that, vectorizing the shape shape of the base manifold has the advantage that the iteration of taking the sasaki_metrix (i.e. looking at the tangent bundle of the tangent bundle) does not introduce new dimensions to the shape.
|
@ambellan, for deep source I don't know if it is possible to run locally (I couldn't find a way by going through their website). Do you know if it is possible @ninamiolane? About the docs, can you provide the error For the tests, the best is to use the flag |
|
I don't know if DeepSource can be run locally, they do have a CLI https://deepsource.io/docs/cli/usage/#commands but it seems to do something different. :/ Yet, note that DeepSource's GitHub workflow usually completes before all of the others, taking ~2min to complete, thus it is not too too long. |
|
@LPereira95 regarding sphinx: I got the following errors: |
|
Dear @LPereira95, for now I incorporated as many of your suggestions as to me seem to be reasonable/feasible. Please take a look at my changes and comments. Thank you for all your valuable remarks, I think they are really helpful to further develop the code. However, there is one thing I don't understand: Why don't you accept any additional comments? I think they usually simply clarify what happens in the code. |
|
General note: Apart form |
|
@LPereira95 and @ninamiolane, I think I addressed all requested changes. Please have a look and iterate/accept the PR. |
|
@ambellan Many thanks!! There is an issue with tensorflow, see here: https://github.com/geomstats/geomstats/runs/7131533833?check_suite_focus=true Would you know what is happening? Good to merge on my side otherwise! |
May be of your interest @nanjekyejoannah. |
There was a problem hiding this comment.
Thanks a lot for addressing the requested changes @ambellan! The code is in great shape now. I'm also very pleased by your effort in creating the gradient document.
Can you please address these last comments?
(If you feel we are iterating too much, let me know and I'll address them myself)
|
@ninamiolane and @LPereira95, thank your for all your valuable feedback. I think I handled all comments raised so far. Please let me know if there is something more left open, or else please accept the PR. |
geomstats/geometry/sasaki_metric.py
Outdated
| self.metric = metric | ||
| shape = (2, gs.prod(gs.array(metric.shape))) | ||
|
|
||
| self.n_jobs = ( |
There was a problem hiding this comment.
Pass n_jobs as input with default value 1 (for consistency with other uses of joblib in the library).
|
Thanks @ambellan. Please address the last comment from my side. Also, can you please try out |
|
@LPereira95, I tested |
|
Thanks a lot @ambellan! I agree with the idea of addressing the additional test in another PR. Looking forward to it! |
|
Thank you both!! @LPereira95 @ambellan |
|
@ninamiolane and @LPereira95, thank you both again for handling the PR! I learned a lot about Life, the Universe, and Everything (well, and of course about Geomstats and good code):grinning:. |
Checklist
and I made sure the documentation renders properly.(Link) (Code is documented, but I cannot get sphinx-build docs/ docs/html running, as it fails immediately.)Description
A natural choice of metric on the tangent bundle of a Riemannian manifold is the Sasaki metric and this PR implements it.
Additional context
This implementation was originally part of the submission 'Sasaki Metric and Applications in Geodesic Analysis' to the ICLR Computational Geometry & Topology Challenge 2022.