Opened 5 months ago

Closed 4 months ago

#30062 closed enhancement (fixed)

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

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.2
Component: categories Keywords:
Cc: tscrim, egourgoulhon Merged in:
Authors: Eric Gourgoulhon, Matthias Koeppe Reviewers: Matthias Koeppe, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: d785c0d (Commits) Commit: d785c0d05c92d9ea96cdaa63df9f52824fa03ce3
Dependencies: Stopgaps:

Description (last modified by 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).

Change History (37)

comment:1 Changed 5 months ago by mkoeppe

  • Cc egourgoulhon added

comment:2 Changed 5 months ago by mkoeppe

  • Description modified (diff)

comment:3 Changed 5 months ago by egourgoulhon

  • Branch set to public/manifolds/euclidean_metric_space
  • Commit set to 1f4f9422e393844eb2edbcbd59b8a8b0b4f5daa4

New commits:

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

comment:4 follow-up: Changed 5 months ago by 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).

comment:5 follow-up: Changed 5 months ago by 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?

comment:6 follow-up: Changed 5 months ago by 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.

comment:7 in reply to: ↑ 4 Changed 5 months ago by 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?

Version 0, edited 5 months ago by mkoeppe (next)

comment:8 in reply to: ↑ 5 Changed 5 months ago by 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.

comment:9 in reply to: ↑ 6 Changed 5 months ago by 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.

comment:10 follow-ups: Changed 5 months ago by 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.

comment:11 in reply to: ↑ 10 Changed 5 months ago by 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.

comment:12 in reply to: ↑ 10 ; follow-up: Changed 5 months ago by 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).

comment:13 in reply to: ↑ 12 Changed 5 months ago by 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.

comment:14 Changed 5 months ago by 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?

comment:15 Changed 5 months ago by git

  • Commit changed from 1f4f9422e393844eb2edbcbd59b8a8b0b4f5daa4 to 7a92b60ad53cc25a20ea86e29ef82fb52f2babc7

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

7a92b60sage.categories.metric_spaces.MetricSpaces: Rename metric to metric_function with deprecation

comment:16 in reply to: ↑ 10 Changed 5 months ago by 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.

comment:17 Changed 4 months ago by mkoeppe

  • Description modified (diff)
  • Summary changed from Add to category of metric spaces to Rename MetricSpaces parent method metric to metric_function, add EuclideanSpace to category of metric spaces

comment:18 Changed 4 months ago by mkoeppe

  • Authors set to Eric Gourgoulhon, Matthias Koeppe
  • Status changed from new to needs_review

comment:19 Changed 4 months ago by tscrim

  • Reviewers set to 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 4 months ago by 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

comment:21 Changed 4 months ago by mkoeppe

  • Status changed from needs_review to needs_work

comment:22 follow-up: Changed 4 months ago by 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?

comment:23 Changed 4 months ago by mkoeppe

  • Status changed from needs_work to needs_info

comment:24 Changed 4 months ago by git

  • Commit changed from 7a92b60ad53cc25a20ea86e29ef82fb52f2babc7 to d99aedae454ab73e0a587fd661609e6e3046bd4b

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

4ec6e62Merge tag '9.2.beta6' into t/30062/public/manifolds/euclidean_metric_space
d99aedasrc/sage/rings/padics/padic_base_leaves.py: Update use of _test_metric

comment:25 Changed 4 months ago by git

  • Commit changed from d99aedae454ab73e0a587fd661609e6e3046bd4b to a1389d8b139b76a4b284b746e69f845da25a4361

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

93facadsrc/sage/structure/category_object.pyx: Update use of _test_metric
a1389d8src/doc/en/thematic_tutorials/vector_calculus: Update categories in doctest

comment:26 Changed 4 months ago by mkoeppe

  • Status changed from needs_info to needs_review

comment:27 in reply to: ↑ 22 Changed 4 months ago by 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()

comment:28 follow-ups: Changed 4 months ago by mkoeppe

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 4 months ago by 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.

comment:30 in reply to: ↑ 28 Changed 4 months ago by 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().

comment:31 follow-up: Changed 4 months ago by 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.

comment:32 in reply to: ↑ 31 Changed 4 months ago by 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).

Last edited 4 months ago by egourgoulhon (previous) (diff)

comment:33 Changed 4 months ago by git

  • Commit changed from a1389d8b139b76a4b284b746e69f845da25a4361 to d785c0d05c92d9ea96cdaa63df9f52824fa03ce3

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

d785c0dBetter category for Euclidean spaces

comment:34 Changed 4 months ago by mkoeppe

  • Reviewers changed from Travis Scrimshaw to Matthias Koeppe, Travis Scrimshaw

comment:35 Changed 4 months ago by 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.)

comment:36 Changed 4 months ago by tscrim

  • Status changed from needs_review to positive_review

Thank you.

comment:37 Changed 4 months ago by vbraun

  • Branch changed from public/manifolds/euclidean_metric_space to d785c0d05c92d9ea96cdaa63df9f52824fa03ce3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.