Opened 2 years ago

Closed 2 years ago

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

Reported by: Owned by: Matthias Köppe major sage-9.2 categories Travis Scrimshaw, Eric Gourgoulhon Eric Gourgoulhon, Matthias Koeppe Matthias Koeppe, Travis Scrimshaw N/A d785c0d d785c0d05c92d9ea96cdaa63df9f52824fa03ce3

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
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).

### comment:2 Changed 2 years ago by Matthias Köppe

Description: modified (diff)

### comment:3 Changed 2 years ago by Eric Gourgoulhon

Branch: → public/manifolds/euclidean_metric_space → 1f4f9422e393844eb2edbcbd59b8a8b0b4f5daa4

New commits:

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

### comment:4 follow-up:  7 Changed 2 years ago by Eric Gourgoulhon

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).

### comment:5 follow-up:  8 Changed 2 years ago by Eric Gourgoulhon

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?

### comment:6 follow-up:  9 Changed 2 years ago by Eric Gourgoulhon

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.

### comment:7 in reply to:  4 Changed 2 years ago by Matthias Köppe

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?

Last edited 2 years ago by Matthias Köppe (previous) (diff)

### comment:8 in reply to:  5 Changed 2 years ago by Matthias Köppe

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.

### comment:9 in reply to:  6 Changed 2 years ago by Matthias Köppe

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.

### comment:10 follow-ups:  11  12  16 Changed 2 years ago by Travis Scrimshaw

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.

### comment:11 in reply to:  10 Changed 2 years ago by Matthias Köppe

Just a quick note:

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.

### comment:12 in reply to:  10 ; follow-up:  13 Changed 2 years ago by Eric Gourgoulhon

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).

### comment:13 in reply to:  12 Changed 2 years ago by Matthias Köppe

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.

### comment:14 Changed 2 years ago by Travis Scrimshaw

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?

### comment:15 Changed 2 years ago by 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

### comment:16 in reply to:  10 Changed 2 years ago by Matthias Köppe

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.

### comment:17 Changed 2 years ago by Matthias Köppe

Description: modified (diff) Add to category of metric spaces → Rename MetricSpaces parent method metric to metric_function, add EuclideanSpace to category of metric spaces

### comment:18 Changed 2 years ago by Matthias Köppe

Authors: → Eric Gourgoulhon, Matthias Koeppe new → needs_review

### comment:19 Changed 2 years ago by Travis Scrimshaw

Reviewers: → Travis Scrimshaw

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.

### comment:20 Changed 2 years ago by Matthias Köppe

Some test failures that look like this

**********************************************************************
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)
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

### comment:21 Changed 2 years ago by Matthias Köppe

Status: needs_review → needs_work

### comment:22 follow-up:  27 Changed 2 years ago by Matthias Köppe

**********************************************************************
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?

### comment:23 Changed 2 years ago by Matthias Köppe

Status: needs_work → needs_info

### comment:24 Changed 2 years ago by 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

### comment:25 Changed 2 years ago by git

Commit: d99aedae454ab73e0a587fd661609e6e3046bd4b → a1389d8b139b76a4b284b746e69f845da25a4361

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

### comment:26 Changed 2 years ago by Matthias Köppe

Status: needs_info → needs_review

### comment:27 in reply to:  22 Changed 2 years ago by Travis Scrimshaw

**********************************************************************
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()

### comment:28 follow-ups:  29  30 Changed 2 years ago by Matthias Köppe

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

### comment:29 in reply to:  28 Changed 2 years ago by Travis Scrimshaw

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.

### comment:30 in reply to:  28 Changed 2 years ago by Eric Gourgoulhon

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().

### comment:31 follow-up:  32 Changed 2 years ago by Eric Gourgoulhon

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.

### comment:32 in reply to:  31 Changed 2 years ago by Eric Gourgoulhon

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).

Last edited 2 years ago by Eric Gourgoulhon (previous) (diff)

### comment:33 Changed 2 years ago by git

Commit: a1389d8b139b76a4b284b746e69f845da25a4361 → d785c0d05c92d9ea96cdaa63df9f52824fa03ce3

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

 ​d785c0d Better category for Euclidean spaces

### comment:34 Changed 2 years ago by Matthias Köppe

Reviewers: Travis Scrimshaw → Matthias Koeppe, Travis Scrimshaw

### comment:35 Changed 2 years ago by Matthias Köppe

Looks great to me, thanks very much.

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

### comment:36 Changed 2 years ago by Travis Scrimshaw

Status: needs_review → positive_review

Thank you.

### comment:37 Changed 2 years ago by Volker Braun

Branch: public/manifolds/euclidean_metric_space → d785c0d05c92d9ea96cdaa63df9f52824fa03ce3 → fixed positive_review → closed
Note: See TracTickets for help on using tickets.