Opened 2 years ago
Closed 2 years 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, GitHub, GitLab) | Commit: | d785c0d05c92d9ea96cdaa63df9f52824fa03ce3 |
Dependencies: | Stopgaps: |
Description (last modified by )
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 2 years ago by
- Cc egourgoulhon added
comment:2 Changed 2 years ago by
- Description modified (diff)
comment:3 Changed 2 years ago by
- Branch set to public/manifolds/euclidean_metric_space
- Commit set to 1f4f9422e393844eb2edbcbd59b8a8b0b4f5daa4
comment:4 follow-up: ↓ 7 Changed 2 years ago by
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()
ofEuclideanSpace
, which is inheritated fromPseudoRiemannianManifold
and which returns the metric tensor (first fundamental form) - the method
metric()
ofParentMethods
inMetricSpaces
, 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
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
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
Replying to egourgoulhon:
The above commit is a first attempt to add
EuclideanSpace
to the category of metric spaces. It endows the classEuclideanSpace
with the methoddist()
, 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()
ofEuclideanSpace
, which is inheritated fromPseudoRiemannianManifold
and which returns the metric tensor (first fundamental form)- the method
metric()
ofParentMethods
inMetricSpaces
, 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()
ofMetricSpaces
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?
comment:8 in reply to: ↑ 5 Changed 2 years ago by
Replying to egourgoulhon:
Another remark about
MetricSpaces
: it defines an element methodabs()
, which returns the distance to the zero element. ForEuclideanSpace
, 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
Replying to egourgoulhon:
Regarding the addition of Riemannian manifolds beyond
EuclideanSpace
to the categoryMetricSpaces
, 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
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
Just a quick note:
Replying to tscrim:
I am -1 for removing the element and parent methods
dist
and themetric
method.a.dist(b)
is natural and easier to work with when you don't want to explicitly specify the parent ofa
(andb
).
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
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 functiond: 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
withmetric_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
Replying to egourgoulhon:
Another option is to replace
metric
bymetric_function
inMetricSpaces
. I would favor that one. From a teaching perspective, it is as easy to discover by TAB completion asmetric
(contrary todistance_function
, which I proposed in comment:4). Moreover, theMetricSpaces
'smetric()
is much less used in Sage's code than the manifold one:grep -r "\.metric()"
insrc/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
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
- Commit changed from 1f4f9422e393844eb2edbcbd59b8a8b0b4f5daa4 to 7a92b60ad53cc25a20ea86e29ef82fb52f2babc7
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
comment:17 Changed 2 years ago by
- 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 2 years ago by
- Status changed from new to needs_review
comment:19 Changed 2 years ago by
- 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 2 years ago by
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 2 years ago by
- Status changed from needs_review to needs_work
comment:22 follow-up: ↓ 27 Changed 2 years ago by
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 2 years ago by
- Status changed from needs_work to needs_info
comment:24 Changed 2 years ago by
- Commit changed from 7a92b60ad53cc25a20ea86e29ef82fb52f2babc7 to d99aedae454ab73e0a587fd661609e6e3046bd4b
comment:25 Changed 2 years ago by
- Commit changed from d99aedae454ab73e0a587fd661609e6e3046bd4b to a1389d8b139b76a4b284b746e69f845da25a4361
comment:26 Changed 2 years ago by
- Status changed from needs_info to needs_review
comment:27 in reply to: ↑ 22 Changed 2 years ago by
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 spacesIs 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
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
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 2 years ago by
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: ↓ 32 Changed 2 years ago by
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
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).
comment:33 Changed 2 years ago by
- Commit changed from a1389d8b139b76a4b284b746e69f845da25a4361 to 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
- Reviewers changed from Travis Scrimshaw to Matthias Koeppe, Travis Scrimshaw
comment:35 Changed 2 years ago by
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:37 Changed 2 years ago by
- Branch changed from public/manifolds/euclidean_metric_space to d785c0d05c92d9ea96cdaa63df9f52824fa03ce3
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
Provide EuclideanSpace with dist() method and add it to category of metric space