Respect docstring conventions laid out here: https://geomstats.github.io/contributing/index.html#writing-docstrings

Eg: Imperativ form for verbs --> Define not Defines
Put verb directly after """

docstring missing parameter description (see other classes' docstring in geomstats)

please write docstring even for private functions

docstring missing parameter descriptions

Give reference of the paper you mention in each docstring, using

References

(see in other places in geomstats for how References is used)

Also, instead of giving the page where we should look for the equation, just write the equation in the docstring, and start the docstring by r"""

--> on the website, the docstring will show the equation in latex!

Eg. https://geomstats.github.io/api/geomstats.information_geometry.html#geomstats.information_geometry.fisher_rao_metric.FisherRaoMetric

Can the docstring also explain the difference between this metric and the Fisher-Rao metric?

And maybe add a section in the docstring:

See Also

FisherRaoMetric

Ditto

Are these the Christoffel symbols associated with the metric_matrix?

• If yes, then they should just be called christoffels.
• If not, then let's create class DivergenceConnection(Connection) in this file, with a christoffels method using the code that you have here, and have self.divergence_connection = DivergenceConnection(...) be an attribute of the StatisticalMetric class.

Doing it like this will allow us to have directly access to the geodesic equation associated with these christoffel symbols! See code below:

https://github.com/geomstats/geomstats/blob/main/geomstats/geometry/connection.py

I believe they are not, they are the Riemannian Christoffell symbols of the two conjugate connections. Agree with the suggested naming convention of class DivergenceConnection(Connection), to be changed from DivergenceConjugate(Connection).

So, I think here the main point is that any of these connections is also an (affine) connection when alone, so we definitely need to break this class into two (e.g. DivergenceConnection, DualDivergenceConnection). Then, by using christoffels as method name, we will have access to all the machinery already implemented in connection (e.g. geodesic_equation, exp, log, etc), as @ninamiolane suggested.

Then, we can always create appropriate objects that combine these (affine) connections (not by inheritance, but by composition). For example, the AlphaConnection can be created by:

class AlphaConnection(Connection):
    def __init__(
        self,
        space,
        primal_connection,
        dual_connection,
        alpha=0,
    ):
        super().__init__(space)
        self.primal_connection = primal_connection
        self.dual_connection = dual_connection
        self.alpha = alpha 

    def christoffels(self, point):
        # see eq. 53
        first_term = (1 + self.alpha) / 2 * self.primal_connection.christoffels(point)
        second_term = (1 - self.alpha) / 2 * self.dual_connection.christoffels(point)

        return first_term - second_term

Alternatively, we could create the AlphaConnection by using the concepts of mean connection (which is itself a connection resulting from a combination of connections) and Amari-Chentsov tensor (eq. 52).

We probably still need to iterate on this, but I believe this AlphaConnection or some kind of ConjugateConnectionPair is the central object here, as e.g. the Amari-Chentsov tensor or other quantities required to define the statistical manifold can be computed from this pair.

For the divergence case we can always create a DivergenceAlphaConnection/DivergenceInducedConnection later (for usability) that creates the necessary connections given a divergence.

The main message is that there's a lot of ways of defining all of this and we probably need to keep the idea of composing objects in the back of our mind all the time.

The reason why I thought it might be important for the divergence induced connection and the dual divergence induced connection to be in the same class is that they both need to be induced by the same divergence, in order for them to have the property of being conjugate connections.

But I also see what you mean -- with each of these connections the user should be able to create geodesics and use the other functionality of connections, so I agree it makes sense to split them up. Is there some way of ensuring that when one is instantiated, the other is automatically instantiated with the same divergence, or that they are linked in some way? Maybe have the AlphaConnection take in a divergence, and then instantiate DivergenceConnection and DualDivergenceConnection in the __init__ method of the AlphaConnection class?

Ditto

Agreed, a creating a class DualDivergenceConnection(Connection) for this will make it consistent then.

I believe they are not, they are the Riemannian Christoffell symbols of the two conjugate connections. Agree with the suggested naming convention of class DivergenceConnection(Connection), to be changed from DivergenceConjugate(Connection).

Agreed, a creating a class DualDivergenceConnection(Connection) for this will make it consistent then.

typo , Instantiated.

update to new naming conventions, using classes

create it as a function (which also receives dim) in order to reuse it in StatisticalMetric.

Just to clarify, this should just be a function outside of any class that is used throughout the script?

I would make this private, as this is just a way of creating this object without having to inherit. An alternative would be to directly implement PotentialFunction as class and override __call__ method.

Delete? having the methods is enough.

will need to find a better name for x.

receive x and return result, not function. (Imagine we make an non autodiff-based implementation of the hessian)

Same as for hessian: don't mind repeating gs.autodiff.hessian(self.potential_function) again.