Rename MetricSpaces parent method metric to metric_function, add EuclideanSpace to category of metric spaces

[mkoeppe]

sage: from sage.categories.metric_spaces import MetricSpaces
sage: QQ in MetricSpaces()
True
sage: RR in MetricSpaces()
True
sage: ZZ in MetricSpaces()
True
sage: AA in MetricSpaces()
False
sage: QuadraticField(5) in MetricSpaces()
False
sage: M = Matrix(QQ,[[2,1,0],[1,2,1],[0,1,2]])
sage: V = VectorSpace(QQ,3,inner_product_matrix=M)
sage: V in MetricSpaces()
False
sage: E = EuclideanSpace(3)
sage: E in MetricSpaces()
False

On this ticket, we add (from ​https://trac.sagemath.org/ticket/30061#comment:44) EuclideanSpace (implementing the distance function by means of the Cartesian chart) to the category.

To do this, we rename the MetricSpaces parent method metric to metric_function.

The other examples from above will be taken care of in #30092 (which adds inner product spaces) and #30219 (real embedded number fields).

[egourgoulhon]

New commits:

​1f4f942

Provide EuclideanSpace with dist() method and add it to category of metric space

[egourgoulhon]

The above commit is a first attempt to add EuclideanSpace to the category of metric spaces. It endows the class EuclideanSpace with the method dist(), which computes the Euclidean distance between two points. However, TestSuite(E).run() fails with _test_metric because of the name clash between

• the method metric() of EuclideanSpace, which is inheritated from PseudoRiemannianManifold and which returns the metric tensor (first fundamental form)
• the method metric() of ParentMethods in MetricSpaces, which is supposed to be the distance function.

It seems difficult to change the name of PseudoRiemannianManifold.metric(), even with a proper deprecation notice, because it is heavily used and it is the natural name in the context of differential geometry. On the other side, could the parent method metric() of MetricSpaces be changed to something else? e.g. distance_function? Also, it seems quite redundant with the other parent method dist(). Indeed, by default, the parent method metric() is defined as

        def metric(self):
            return lambda a,b: a.dist(b)

with a.dist(b) being self.parent().dist(self, b), so at the end of the day, if M is a metric space, M.metric()(a,b) ends to M.dist(a,b).

[egourgoulhon]

Another remark about MetricSpaces: it defines an element method abs(), which returns the distance to the zero element. For EuclideanSpace, the zero element could be defined as the element of Cartesian coordinates (0,0,...,0), but in general, metric spaces (which are not necessarily algebraic structures) do not have any natural "zero" element, do they?

[egourgoulhon]

Regarding the addition of Riemannian manifolds beyond EuclideanSpace to the category MetricSpaces, this seems computationaly difficult. It is true that any connected Riemannian manifold can be turned into a metric space by defining the distance function as the infinimum of the lengths of the paths connecting two points, but the latter is not computable in practice.

[mkoeppe]

Replying to egourgoulhon:

The above commit is a first attempt to add EuclideanSpace to the category of metric spaces. It endows the class EuclideanSpace with the method dist(), which computes the Euclidean distance between two points.

Great!

However, TestSuite(E).run() fails with _test_metric because of the name clash between

• the method metric() of EuclideanSpace, which is inheritated from PseudoRiemannianManifold and which returns the metric tensor (first fundamental form)
• the method metric() of ParentMethods in MetricSpaces, which is supposed to be the distance function.

It seems difficult to change the name of PseudoRiemannianManifold.metric(), even with a proper deprecation notice, because it is heavily used and it is the natural name in the context of differential geometry.

I agree.

On the other side, could the parent method metric() of MetricSpaces be changed to something else?

Overall, this category seems not very well developed, and I would be surprised if it was used much in user code. I think it will be safe to make changes to it (with deprecation).

e.g. distance_function?

Sounds good to me. But perhaps we can just deprecate the parent method metric altogether until a clear need for it arises?

[mkoeppe]

Replying to egourgoulhon:

Another remark about MetricSpaces: it defines an element method abs(), which returns the distance to the zero element. For EuclideanSpace, the zero element could be defined as the element of Cartesian coordinates (0,0,...,0), but in general, metric spaces (which are not necessarily algebraic structures) do not have any natural "zero" element, do they?

I would propose to deprecate this element method, exactly for this reason. Probably when the category was written, the author was thinking of normed spaces; but these should become a separate category.

[mkoeppe]

Replying to egourgoulhon:

Regarding the addition of Riemannian manifolds beyond EuclideanSpace to the category MetricSpaces, this seems computationaly difficult. It is true that any connected Riemannian manifold can be turned into a metric space by defining the distance function as the infinimum of the lengths of the paths connecting two points, but the latter is not computable in practice.

Good point about the needed hypothesis of connectedness. That alone is already a good enough reason not to add Riemannian manifolds to the category. So we can ignore the more subtle question whether it makes sense to add it to the category but have the dist method raise NotImplementedError for nontrivial inputs.

[tscrim]

Indeed, I was conflating metric spaces and normed spaces, which should be separated into 2 separate categories (with normed spaces being a subcategory of metric spaces of course).

Just to note: We do have an axiom for connected and the category Manifolds().Connected().

I am -1 for removing the element and parent methods dist and the metric method. a.dist(b) is natural and easier to work with when you don't want to explicitly specify the parent of a (and b). It is also more conceptual when you don't want to care about the exact metric as long as there is some metric. Similarly, when you want to specify which metric (space) you want to use M.dist(a,b) or M.metric()(a,b) becomes the more natural choice. Additionally, sometimes you want to explicitly work or pass the metric, so M.metric() is something useful too. There should not be any conflict here and having extra (natural) methods doesn't hurt anything.

I would also be wary of saying there is no need just because something is not used in the Sage code: It could be used in a user's code and it feels natural from a teaching perspective. Additionally, me being the (dumb) non-geometer that I am, I would expect asking for the metric() of a metric space to return the metric function d: X x X -> R.

As Eric said in comment:4, the real issue is the name conflict. From some wikipedia searching, what is currently called a metric in the manifolds code is really a (very common) shorthand name for "metric tensor," whereas "metric" is the standard name for the function that defines a metric space. Of course, the metric tensor allows one to define a metric as Eric said, and it is clear from the context of the different fields what one means. Unfortunately there is not a good way to resolve this other than possibly replacing metric with metric_tensor in the manifolds code. However, I think it is reasonable to not put the connected Riemannian manifolds in the MetricSpaces category because of the computational infeasibility of the metric function.

[mkoeppe]

Just a quick note:

Replying to tscrim:

I am -1 for removing the element and parent methods dist and the metric method. a.dist(b) is natural and easier to work with when you don't want to explicitly specify the parent of a (and b).

There is no problem with the element method dist. The only problem is the clash of the parent method metric.

[egourgoulhon]

Replying to tscrim:

I would also be wary of saying there is no need just because something is not used in the Sage code: It could be used in a user's code and it feels natural from a teaching perspective. Additionally, me being the (dumb) non-geometer that I am, I would expect asking for the metric() of a metric space to return the metric function d: X x X -> R.

I agree this quite natural.

As Eric said in comment:4, the real issue is the name conflict. From some wikipedia searching, what is currently called a metric in the manifolds code is really a (very common) shorthand name for "metric tensor," whereas "metric" is the standard name for the function that defines a metric space. Of course, the metric tensor allows one to define a metric as Eric said, and it is clear from the context of the different fields what one means.

Indeed, both usages of metric are natural in their respective contexts.

Unfortunately there is not a good way to resolve this other than possibly replacing metric with metric_tensor in the manifolds code.

Another option is to replace metric by metric_function in MetricSpaces. I would favor that one. From a teaching perspective, it is as easy to discover by TAB completion as metric (contrary to distance_function, which I proposed in comment:4). Moreover, the MetricSpaces's metric() is much less used in Sage's code than the manifold one: grep -r "\.metric()" in src/sage returns 5 lines for metric spaces, versus 127 lines for manifolds.

However, I think it is reasonable to not put the connected Riemannian manifolds in the MetricSpaces category because of the computational infeasibility of the metric function.

I agree, but this does not solve the clash issue for EuclideanSpace, since the latter is considered as a Riemannian manifold (with a flat metric tensor).

[mkoeppe]

Replying to egourgoulhon:

Another option is to replace metric by metric_function in MetricSpaces. I would favor that one. From a teaching perspective, it is as easy to discover by TAB completion as metric (contrary to distance_function, which I proposed in comment:4). Moreover, the MetricSpaces's metric() is much less used in Sage's code than the manifold one: grep -r "\.metric()" in src/sage returns 5 lines for metric spaces, versus 127 lines for manifolds.

+1 on metric_function (with deprecation for metric) in MetricSpaces. I think this is a good compromise.

[tscrim]

I agree that renaming metric() -> metric_function() and using that internally is good. However, I might also advocate for doing metric() -> metric_tensor() in the manifold code as it is basically just one sed command, as well as providing metric() as a shorthand.

The slightly trickier question would be should we allow MetricSpaces to have metric() be a shorthand whose semantics could change?

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​7a92b60

sage.categories.metric_spaces.MetricSpaces: Rename metric to metric_function with deprecation

[mkoeppe]

Replying to tscrim:

Indeed, I was conflating metric spaces and normed spaces, which should be separated into 2 separate categories (with normed spaces being a subcategory of metric spaces of course).

Let's fix this in #30092.

[tscrim]

While I am not fully convinced that we should not have an ambiguous metric method, it is not a strong enough opinion to warrant asking for further discussion. So if a patchbot comes back (morally) green, then positive review.

[mkoeppe]

Some test failures that look like this

sage -t --long src/sage/rings/padics/padic_base_leaves.py
**********************************************************************
File "src/sage/rings/padics/padic_base_leaves.py", line 237, in sage.rings.padics.padic_base_leaves.pAdicRingCappedRelative.__init__
Failed example:
    R._test_metric(elements = [R.random_element() for i in range(2^3)]) # long time
Exception raised:
    Traceback (most recent call last):
      File "sage/structure/category_object.pyx", line 838, in sage.structure.category_object.CategoryObject.getattr_from_category (build/cythonized/sage/structure/category_object.c:6954)
        return self.__cached_methods[name]
    KeyError: '_test_metric'

    During handling of the above exception, another exception occurred:

    Traceback (most recent call last):
      File "/home/sagemath/sage-9.1/local/lib/python3.7/site-packages/sage/doctest/forker.py", line 707, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/sagemath/sage-9.1/local/lib/python3.7/site-packages/sage/doctest/forker.py", line 1131, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.rings.padics.padic_base_leaves.pAdicRingCappedRelative.__init__[5]>", line 1, in <module>
        R._test_metric(elements = [R.random_element() for i in range(Integer(2)**Integer(3))]) # long time
      File "sage/structure/category_object.pyx", line 832, in sage.structure.category_object.CategoryObject.__getattr__ (build/cythonized/sage/structure/category_object.c:6876)
        return self.getattr_from_category(name)
      File "sage/structure/category_object.pyx", line 847, in sage.structure.category_object.CategoryObject.getattr_from_category (build/cythonized/sage/structure/category_object.c:7039)
        attr = getattr_from_other_class(self, cls, name)
      File "sage/cpython/getattr.pyx", line 389, in sage.cpython.getattr.getattr_from_other_class (build/cythonized/sage/cpython/getattr.c:2548)
        raise AttributeError(dummy_error_message)
    AttributeError: 'pAdicRingCappedRelative_with_category' object has no attribute '_test_metric'
**********************************************************************

... and some others

[mkoeppe]

sage -t --long src/doc/en/thematic_tutorials/vector_calculus/vector_calc_advanced.rst
**********************************************************************
File "src/doc/en/thematic_tutorials/vector_calculus/vector_calc_advanced.rst", line 31, in doc.en.thematic_tutorials.vector_calculus.vector_calc_advanced
Failed example:
    E.category()
Expected:
    Category of smooth manifolds over Real Field with 53 bits of precision
Got:
    Join of Category of differentiable manifolds over Real Field with 53 bits of precision and Category of metric spaces

Is the change from "smooth" to "differentiable" expected here?

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​4ec6e62	Merge tag '9.2.beta6' into t/30062/public/manifolds/euclidean_metric_space
​d99aeda	src/sage/rings/padics/padic_base_leaves.py: Update use of _test_metric

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​93facad	src/sage/structure/category_object.pyx: Update use of _test_metric
​a1389d8	src/doc/en/thematic_tutorials/vector_calculus: Update categories in doctest

[tscrim]

Replying to mkoeppe:

sage -t --long src/doc/en/thematic_tutorials/vector_calculus/vector_calc_advanced.rst
**********************************************************************
File "src/doc/en/thematic_tutorials/vector_calculus/vector_calc_advanced.rst", line 31, in doc.en.thematic_tutorials.vector_calculus.vector_calc_advanced
Failed example:
    E.category()
Expected:
    Category of smooth manifolds over Real Field with 53 bits of precision
Got:
    Join of Category of differentiable manifolds over Real Field with 53 bits of precision and Category of metric spaces

Is the change from "smooth" to "differentiable" expected here?

You explicitly changed the default category to only be differentiable rather than smooth:

+        if category is None:
+            category = Manifolds(RR).Differentiable() & MetricSpaces()

See sage.manifolds.differentiable.manifold.DifferentiableManifold.__init__. So I think you need to change

-            category = Manifolds(RR).Differentiable() & MetricSpaces()
+            category = Manifolds(RR).Smooth() & MetricSpaces()

[mkoeppe]

Eric's commit 1f4f9422 did this, so I guess my question is to him whether this change was intentional

[tscrim]

Replying to mkoeppe:

Eric's commit 1f4f9422 did this, so I guess my question is to him whether this change was intentional

Ah, sorry, I thought it was on one of your commits.

[egourgoulhon]

Replying to mkoeppe:

Eric's commit 1f4f9422 did this, so I guess my question is to him whether this change was intentional

Sorry it is a mistake from my side; of course, for Euclidean spaces, the category shall be Manifolds(RR).Smooth().

[egourgoulhon]

Since we are on it, we could also add that the Euclidean space is in the category of complete metric spaces:

    category = Manifolds(RR).Smooth() & MetricSpaces().Complete()

with hopefully some day RR replaced by a better representation of the real field, as proposed in #24456.

[egourgoulhon]

Replying to egourgoulhon:

Since we are on it, we could also add that the Euclidean space is in the category of complete metric spaces:

    category = Manifolds(RR).Smooth() & MetricSpaces().Complete()

with hopefully some day RR replaced by a better representation of the real field, as proposed in #24456.

I'll add a commit implementing this soon (EDIT: this is the commit in comment:33).

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​d785c0d

Better category for Euclidean spaces

[mkoeppe]

Looks great to me, thanks very much.

Patchbot is green too. (There is one patchbot that is not feeling well, unrelated to this ticket.)

[tscrim]

Thank you.