Opened 5 years ago
Last modified 4 years ago
#18529 closed enhancement
Topological manifolds: basics — at Version 44
Reported by:  egourgoulhon  Owned by:  egourgoulhon 

Priority:  major  Milestone:  sage7.1 
Component:  geometry  Keywords:  topological manifolds 
Cc:  mmancini  Merged in:  
Authors:  Eric Gourgoulhon  Reviewers:  
Report Upstream:  N/A  Work issues:  
Branch:  public/manifolds/top_manif_basics  Commit:  902908b41a95d3455bfcc497997ad2054c530a96 
Dependencies:  #18175  Stopgaps: 
Description (last modified by )
This is the implementation of topological manifolds over a topological field K resulting from the SageManifolds project. See the metaticket #18528 for an overview. By topological manifold over a topological field K it is meant a second countable Hausdorff space M such that every point in M has a neighborhood homeomorphic to K^{n}, with the same nonnegative integer n for all points.
This tickets implements the following Python classes:
TopologicalManifold
: topological manifold over a topological field KTopologicalManifoldPoint
: point in a topological manifoldTopologicalManifoldSubset
: generic subset of a topological manifoldChart
: chart of a topological manifoldRealChart
: chart of a topological manifold over the real field
CoordChange
: transition map between two charts of a topological manifold
TopologicalManifold
is intended to serve as a base class for specific manifolds, like smooth manifolds (K=R) and complex manifolds (K=C).
Documentation:
The reference manual is produced by
sage docbuild reference/manifolds html
It can also be accessed online at http://sagemanifolds.obspm.fr/doc/18529/reference/manifolds/
More documentation (e.g. example worksheets) can be found here.
Change History (44)
comment:1 Changed 5 years ago by
 Description modified (diff)
comment:2 followup: ↓ 4 Changed 5 years ago by
comment:3 Changed 5 years ago by
 Description modified (diff)
comment:4 in reply to: ↑ 2 Changed 5 years ago by
Replying to jhpalmieri:
The phrase "manifold over a field K" sounds odd to me. Is it used in the literature?
Thanks for your comment. You are right: this is an abusive generalization of "manifold over R" and "manifold over C", which are used in the literature.
What if K is a finite field? It seems that if X is a finite discrete space, for every finite field F and for every nonnegative integer n, then X is a manifold over F of dimension n: F and n play no role. (I'm assuming that finite fields have been given the discrete topology.)
I think you might say "topological manifold over a topological field K", since obviously the topology on K is critical. Or you could omit "over a field K", and mention in the documentation that users can specify a topological field (like \CC, rather than the default \RR) if they want.
Thanks for your suggestion; I've modified the ticket description accordingly. I've also added what is meant by "topological manifold over a topological field K".
PS: note that the code in the associated branch is still in a very crude draft state, but should be ready for review within a few days.
comment:5 Changed 5 years ago by
 Commit changed from 89c063c7119f19497e3d21b2a5a9dcb0752122b0 to 5a5722b4a0ef33d8624fdd127bbb1964232ced96
Branch pushed to git repo; I updated commit sha1. New commits:
5a5722b  Add doctests in classes Chart and RealChart

comment:6 Changed 5 years ago by
 Commit changed from 5a5722b4a0ef33d8624fdd127bbb1964232ced96 to 4f490af5fedeb0a28dd8ddab70efdab1cc64bf93
Branch pushed to git repo; I updated commit sha1. New commits:
4f490af  Improve the documentation of coordinate charts

comment:7 Changed 5 years ago by
 Commit changed from 4f490af5fedeb0a28dd8ddab70efdab1cc64bf93 to d8df59f286da79e1e103b56064fdffb702e034ce
Branch pushed to git repo; I updated commit sha1. New commits:
d8df59f  Improve the documentation of TopManifold

comment:8 Changed 5 years ago by
 Commit changed from d8df59f286da79e1e103b56064fdffb702e034ce to fb96562c1e7d4ffc76ca87efcc15d133c6c15190
Branch pushed to git repo; I updated commit sha1. New commits:
fb96562  Open subsets of topological manifolds are now fully considered as topological manifolds.

comment:9 Changed 5 years ago by
 Description modified (diff)
comment:10 Changed 5 years ago by
 Commit changed from fb96562c1e7d4ffc76ca87efcc15d133c6c15190 to 38c3c12cb1d9b4d52a35ad6f7a1a8d0ee666d532
Branch pushed to git repo; I updated commit sha1. New commits:
38c3c12  Improve documentation of classes TopManifold, TopManifoldSubset and TopManifoldPoint

comment:11 Changed 5 years ago by
 Commit changed from 38c3c12cb1d9b4d52a35ad6f7a1a8d0ee666d532 to 7809ebf468f80143e33830e0df75046bf191ce24
Branch pushed to git repo; I updated commit sha1. New commits:
7809ebf  Minor modifications in classes TopManifold and Chart.

comment:12 Changed 5 years ago by
 Commit changed from 7809ebf468f80143e33830e0df75046bf191ce24 to 99cc8c1c94197b1648966eaa7b2a6d04ddab1efc
Branch pushed to git repo; I updated commit sha1. New commits:
99cc8c1  Introduce Chart._init_coordinates() to simplify constructors of Chart and RealChart

comment:13 Changed 5 years ago by
 Commit changed from 99cc8c1c94197b1648966eaa7b2a6d04ddab1efc to 26fc318bbacfc9a27049a9397d43402975525820
Branch pushed to git repo; I updated commit sha1. New commits:
26fc318  Modifications in CoordChange to allow for subclasses.

comment:14 Changed 5 years ago by
 Commit changed from 26fc318bbacfc9a27049a9397d43402975525820 to a74c2e0519c166657b858be5b4b3dc4fd0145d09
Branch pushed to git repo; I updated commit sha1. New commits:
a74c2e0  Add example of padic manifold

comment:15 Changed 5 years ago by
 Commit changed from a74c2e0519c166657b858be5b4b3dc4fd0145d09 to 542b82a9ed134759fe498cd34b682b2d565061c1
Branch pushed to git repo; I updated commit sha1. New commits:
542b82a  Suppress verbose in TestSuite().run; minor improvements in documentation

comment:16 Changed 5 years ago by
 Commit changed from 542b82a9ed134759fe498cd34b682b2d565061c1 to 26c489001c4ee64a95ee70da72210e361180eeb0
Branch pushed to git repo; I updated commit sha1. New commits:
26c4890  Minor improvements in the doc of topological manifolds (basics part)

comment:17 Changed 5 years ago by
 Commit changed from 26c489001c4ee64a95ee70da72210e361180eeb0 to be3ff7424963450c1c2dfa7ca9fbe85eaeab1162
Branch pushed to git repo; I updated commit sha1. New commits:
be3ff74  Introduce open covers on top manifolds

comment:18 Changed 5 years ago by
 Commit changed from be3ff7424963450c1c2dfa7ca9fbe85eaeab1162 to 4de19a74c83ac6d4d0c4da74e1d1f2afce5c3045
Branch pushed to git repo; I updated commit sha1. New commits:
4de19a7  Merge branch 'public/manifolds/top_manif_basics' of git://trac.sagemath.org/sage into sage 6.9

comment:19 Changed 5 years ago by
 Description modified (diff)
 Milestone changed from sage6.8 to sage6.10
 Status changed from new to needs_review
comment:20 followup: ↓ 21 Changed 5 years ago by
Why did you abreviate TopologicalManifold
? Everywhere in Sage classes have plain names like PolynomialRing
and not PolRing
.
comment:21 in reply to: ↑ 20 Changed 5 years ago by
Replying to vdelecroix:
Why did you abreviate
TopologicalManifold
?
No strong argument, except that this is shorter to write (well, thanks to the tab key this is a pretty weak argument) and it is a reminder of Top, which is the standard name for the category of topological manifolds. Similarly, we useDiffManifold
for differentiable manifold, Diff being the standard name of the category of differentiable manifolds.
Everywhere in Sage classes have plain names like
PolynomialRing
and notPolRing
.
This is indeed a strong point: naming conventions should be homogeneous all across Sage. So I am considering to change everywhere TopManifold
to TopologicalManifold
and DiffManifold
to DifferentiableManifold
.
comment:22 Changed 5 years ago by
 Commit changed from 4de19a74c83ac6d4d0c4da74e1d1f2afce5c3045 to 6dec6d592a09e56921b9a761827309dd31ae2533
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
3647305  Some cleanup and adding more category information to particular sets.

f810aa6  Reworking the category of manifolds.

375ff46  Fixing doctest failures and letting a few other rings know they are metric spaces.

8b851a0  Merge branch 'develop' into public/categories/topological_metric_spaces18175

f8f5b93  Fixing last remaining doctests.

041a5d1  Adding padics to metric spaces and some cleanup.

bfa0cdf  One last doc tweak.

d13c368  Fixing doc of metric spaces.

2605c0b  Merge #18529 (Topological manifolds: basics) into #18175 (Implement categories for topological...)

6dec6d5  Implement topological manifolds (basics, #18529) on the new categories for manifolds (#18175)

comment:23 followup: ↓ 24 Changed 5 years ago by
I agree with Vincent here; we should be explicit in our names.
Although is there a reason why you just did not have a single Manifold
function/class as a global entry point, which then could delegate out to *Manifold
depending on some boolean input arguments? I feel that this would make the user interface easier and have a single collective point for general documentation. Another option I see would be to have a manifolds
catalog which then would have things like Topological
, Differentiable
, etc.
comment:24 in reply to: ↑ 23 ; followup: ↓ 25 Changed 5 years ago by
Yes, it is pretty clear that we should have the following renaming of *classes*:
TopManifold
>TopologicalManifold
DiffManifold
>DifferentiableManifold
Regarding the user interface, we could indeed have a *function* Manifold
that looks like
Manifold(dim, symbol, type='smooth', ...)
with 'topological'
, 'differentiable'
, 'smooth'
as possible values for the parameter type
(as well as 'analytic'
, etc. in the future). A question: should 'smooth'
be the default value?
Besides, I remember someone was complaining that Manifold
was already used in SnapPy, cf. this page, but is there any danger of name clash? i.e. is SnapPy used (or could be used in the future) from Sage?
comment:25 in reply to: ↑ 24 ; followup: ↓ 26 Changed 5 years ago by
Replying to egourgoulhon:
Regarding the user interface, we could indeed have a *function*
Manifold
that looks likeManifold(dim, symbol, type='smooth', ...)with
'topological'
,'differentiable'
,'smooth'
as possible values for the parametertype
(as well as'analytic'
, etc. in the future). A question: should'smooth'
be the default value?
I don't know how easy it would be to make this a check on the atlas used to construct the manifold. I would make it select it automatically if possible, and then default to the most general setting of 'topological'
.
Besides, I remember someone was complaining that
Manifold
was already used in SnapPy, cf. this page, but is there any danger of name clash? i.e. is SnapPy used (or could be used in the future) from Sage?
I would then add an additional argument of implementation
, which would default to the native Sage implementation, if there does become a name clash and have the constructor delegate out to the SnapPy version when it exists in Sage (IDK if it is Sage now or not).
comment:26 in reply to: ↑ 25 Changed 5 years ago by
Replying to tscrim:
I don't know how easy it would be to make this a check on the atlas used to construct the manifold. I would make it select it automatically if possible, and then default to the most general setting of
'topological'
.
Actually in the current setting, the atlas is not specified at the construction of the manifold: charts and transition maps are introduced on the fly, at any step in the user workflow. Therefore the type of manifold has to be declared explicitly by the user. It is my impression (but I may be biased) that one speaks about a "manifold" without any qualifier, one implicitely means a "real smooth manifold". Therefore I would set the default arguments of the Manifold
function to field='real'
and type='smooth'
.
I would then add an additional argument of
implementation
, which would default to the native Sage implementation, if there does become a name clash and have the constructor delegate out to the SnapPy version when it exists in Sage (IDK if it is Sage now or not).
Good idea!
comment:27 followup: ↓ 28 Changed 5 years ago by
Some things from looking over the current structure:
 I would separate out parts of the subset class that applies to
Top(ological)Manifold
andTopManifoldSubset
into an ABC (abstract base class) so you don't have to do things likeself is manifold
.
 Maybe put the subsets into the
Subobjects
class and maybe consider not making it a facade?
 Should manifolds and their subsets be
UniqueRepresentation
? I understand that the name as the only input means they are essentially treated as variables, but it means you have to do things likeTopManifold._clear_cache_()
(which may be needed not just necessarily in doctests if one does this before the gc comes around).
 Related to the above, we want parents to be hashable and have a good equality. I know that by making it a
UniqueRepresentation
, many of these issues are solved. However the manifolds are essentially mutable, but by doing a__hash__
which only depends upon the name(s) and dimension means this is okay (__eq__
defaults to check byis
). This would alleviate the need forUniqueRepresentation
.
 In the tests for
_element_constructor_
, you shouldn't call it explicitly but viaX(...)
, which also checks that it is working correctly with the coercion framework.
 I don't quite agree with checking
self._field == RR
for real charts as the precision should not matter. I would checkisinstance(self._field, RealField)
instead. Granted, we probably should have a function that checks against all known real field implementations...
 In a similar vein, I don't like the input of
'real'
toRR
(and'complex'
toCC
). I would make the user be explicit about what they want unless you are doing to do some special handling to make this pass special arguments to an underlyingSR
implementation.
comment:28 in reply to: ↑ 27 ; followups: ↓ 29 ↓ 31 Changed 5 years ago by
Replying to tscrim:
Thanks for your comments/suggestions.
Some things from looking over the current structure:
 I would separate out parts of the subset class that applies to
Top(ological)Manifold
andTopManifoldSubset
into an ABC (abstract base class) so you don't have to do things likeself is manifold
.
I am not sure an ABC would help here: this would make a clear distinction between the manifold and strict subsets of it (thus avoiding the very few tests self is self._manifold
), but on the other hand, we need open strict subsets to be in the class TopologicalManifolds
.
 Maybe put the subsets into the
Subobjects
class and maybe consider not making it a facade?
I had the impression that the facade feature is exactly what we need, since we can have the same point created from different facade parents by means of different charts: if U
and V
are two overelapping open subsets of manifold M
, it may happen that the same point of M
is declared in two different ways:
p = U((x,y), chart=C1) q = V((u,v), chart=C2)
Thanks to the facade mecanism, p
and q
will have the same parent: M
and then it is possible to check whether p == q
; this will hold if the coordinates (x,y)
in chart C1
correspond to coordinates (u,v)
in chart C2
. Is there something bad in using facade parents?
 Should manifolds and their subsets be
UniqueRepresentation
? I understand that the name as the only input means they are essentially treated as variables, but it means you have to do things likeTopManifold._clear_cache_()
(which may be needed not just necessarily in doctests if one does this before the gc comes around).
Actually, I don't see any use case (except for confusing the reader!) where one would define two different manifolds with the same name and same dimension over the same topological field. So UniqueRepresentation
seems quite appropriate here. Moreover, I thought this is Sage's credo to have parents be UniqueRepresentation
, cf. the phrase You are encouraged to make your parent "unique" in the tutorial How to implement new algebraic structures in Sage. Also, most parents in src/sage/geometry
(in particular the hyperbolic plane) have UniqueRepresentation
.
 Related to the above, we want parents to be hashable and have a good equality. I know that by making it a
UniqueRepresentation
, many of these issues are solved.
Indeed!
However the manifolds are essentially mutable, but by doing a
__hash__
which only depends upon the name(s) and dimension means this is okay (__eq__
defaults to check byis
). This would alleviate the need forUniqueRepresentation
.
Why should we avoid UniqueRepresentation
? It's true that manifolds are mutable as Python objects, since the main idea is to add charts (and vector frames for differentiable manifolds) "on the fly" (for, in a concrete calculation, charts are often defined from previous ones by some transition map that might depend on some computational result and therefore it is impossible to introduce all charts at the instantiation of the manifold object). But this mutability is "secondary": the primary attributes of a manifold being its dimension, its name and its base field. With respect to these, the manifold is immutable. The need for _clear_cache_()
in some doctests seems a very small annoyment, with respect to the benefit of UniqueRepresentation
, doesn't it ? (I am afraid I have not understood the nondoctest case involving the garbage collector that you mentioned)
 In the tests for
_element_constructor_
, you shouldn't call it explicitly but viaX(...)
, which also checks that it is working correctly with the coercion framework.
OK, I will change this.
 I don't quite agree with checking
self._field == RR
for real charts as the precision should not matter. I would checkisinstance(self._field, RealField)
instead.
Yes, I agree. I was also not satisfied with this.
Granted, we probably should have a function that checks against all known real field implementations...
 In a similar vein, I don't like the input of
'real'
toRR
(and'complex'
toCC
). I would make the user be explicit about what they want unless you are doing to do some special handling to make this pass special arguments to an underlyingSR
implementation.
Yes, I agree too.
comment:29 in reply to: ↑ 28 Changed 5 years ago by
Replying to egourgoulhon:
Replying to tscrim:
 I would separate out parts of the subset class that applies to
Top(ological)Manifold
andTopManifoldSubset
into an ABC (abstract base class) so you don't have to do things likeself is manifold
.I am not sure an ABC would help here: this would make a clear distinction between the manifold and strict subsets of it (thus avoiding the very few tests
self is self._manifold
), but on the other hand, we need open strict subsets to be in the classTopologicalManifolds
.
I gave a second thought to this: are you thinking about something like
The_ABC / \ TopologicalManifoldStrictSubset TopologicalManifold \ / TopologicalManifoldStrictOpenSubset
with the methods superset(), intersection() and union() being implemented in each of the classes TopologicalManifoldStrictSubset
and TopologicalManifold
?
comment:30 Changed 5 years ago by
In a short word, yes, that is correct. I will respond in more detail tomorrow morning when I wake up.
comment:31 in reply to: ↑ 28 ; followup: ↓ 32 Changed 5 years ago by
Replying to egourgoulhon:
Replying to tscrim:
Thanks for your comments/suggestions.
Some things from looking over the current structure:
 I would separate out parts of the subset class that applies to
Top(ological)Manifold
andTopManifoldSubset
into an ABC (abstract base class) so you don't have to do things likeself is manifold
.I am not sure an ABC would help here: this would make a clear distinction between the manifold and strict subsets of it (thus avoiding the very few tests
self is self._manifold
), but on the other hand, we need open strict subsets to be in the classTopologicalManifolds
.
The diagram you have below is what I was thinking (at least the first 2 levels). This gives better separation of concerns and better distinguishes the two classes.
 Maybe put the subsets into the
Subobjects
class and maybe consider not making it a facade?I had the impression that the facade feature is exactly what we need, since we can have the same point created from different facade parents by means of different charts: if
U
andV
are two overelapping open subsets of manifoldM
, it may happen that the same point ofM
is declared in two different ways:p = U((x,y), chart=C1) q = V((u,v), chart=C2)Thanks to the facade mecanism,
p
andq
will have the same parent:M
and then it is possible to check whetherp == q
; this will hold if the coordinates(x,y)
in chartC1
correspond to coordinates(u,v)
in chartC2
. Is there something bad in using facade parents?
It depends on what you want the points associated with. If they are to be considered proper subsets of the manifold, where you want to do operations, then the point should know that it belongs to the subset. If it is really just reflecting a particular chart, then you probably should reconsider your entire class hierarchy because I think it shouldn't quack like a manifold in that case.
It really comes down to what you're doing mostly in the code. If you'd be doing a lot of conversions between points thought of as being in the subset and as in the manifold, then a facade is probably an okay way to go. If you care about associating the point with a particular subset, then it should not be a facade.
 Should manifolds and their subsets be
UniqueRepresentation
? I understand that the name as the only input means they are essentially treated as variables, but it means you have to do things likeTopManifold._clear_cache_()
(which may be needed not just necessarily in doctests if one does this before the gc comes around).Actually, I don't see any use case (except for confusing the reader!) where one would define two different manifolds with the same name and same dimension over the same topological field. So
UniqueRepresentation
seems quite appropriate here. Moreover, I thought this is Sage's credo to have parents beUniqueRepresentation
, cf. the phrase You are encouraged to make your parent "unique" in the tutorial How to implement new algebraic structures in Sage. Also, most parents insrc/sage/geometry
(in particular the hyperbolic plane) haveUniqueRepresentation
.
The hyperbolic plane works well because it is uniquely defined. Also not all parents are UniqueRepresentations
, the reason why we do this is because of the coercion framework and speed. Identity checks are typically much faster than equality checks.
However the manifolds are essentially mutable, but by doing a
__hash__
which only depends upon the name(s) and dimension means this is okay (__eq__
defaults to check byis
). This would alleviate the need forUniqueRepresentation
.Why should we avoid
UniqueRepresentation
? It's true that manifolds are mutable as Python objects, since the main idea is to add charts (and vector frames for differentiable manifolds) "on the fly" (for, in a concrete calculation, charts are often defined from previous ones by some transition map that might depend on some computational result and therefore it is impossible to introduce all charts at the instantiation of the manifold object). But this mutability is "secondary": the primary attributes of a manifold being its dimension, its name and its base field. With respect to these, the manifold is immutable. The need for_clear_cache_()
in some doctests seems a very small annoyment, with respect to the benefit ofUniqueRepresentation
, doesn't it ? (I am afraid I have not understood the nondoctest case involving the garbage collector that you mentioned)
This comes down to the manifold not being welldefined by its dimension, name, and base field. It also needs to be defined by its chart. I understand that giving two distinct manifolds the same name is bad mathematically, and you are reflecting that in your code. However what if I create a manifold M
, then decide I want to create a new (and different) manifold M
of the same dimension over the same base field? When I try to do this naïvely, I get all of the previous chart information much to my surprise (the garbage collector this was more about temporary objects).
comment:32 in reply to: ↑ 31 ; followup: ↓ 33 Changed 5 years ago by
Replying to tscrim:
It really comes down to what you're doing mostly in the code. If you'd be doing a lot of conversions between points thought of as being in the subset and as in the manifold, then a facade is probably an okay way to go. If you care about associating the point with a particular subset, then it should not be a facade.
I think we are precisely in the first case. In particular we do not care about associating the point with a particular subset (since many overlapping subsets can be introduced that contain the point and none of them is to be privileged with respect to the point). So I would say that we should use the facade.
The hyperbolic plane works well because it is uniquely defined. Also not all parents are
UniqueRepresentations
, the reason why we do this is because of the coercion framework and speed. Identity checks are typically much faster than equality checks.
Thanks for these explanations.
However the manifolds are essentially mutable, but by doing a
__hash__
which only depends upon the name(s) and dimension means this is okay (__eq__
defaults to check byis
). This would alleviate the need forUniqueRepresentation
.Why should we avoid
UniqueRepresentation
? It's true that manifolds are mutable as Python objects, since the main idea is to add charts (and vector frames for differentiable manifolds) "on the fly" (for, in a concrete calculation, charts are often defined from previous ones by some transition map that might depend on some computational result and therefore it is impossible to introduce all charts at the instantiation of the manifold object). But this mutability is "secondary": the primary attributes of a manifold being its dimension, its name and its base field. With respect to these, the manifold is immutable. The need for_clear_cache_()
in some doctests seems a very small annoyment, with respect to the benefit ofUniqueRepresentation
, doesn't it ? (I am afraid I have not understood the nondoctest case involving the garbage collector that you mentioned)This comes down to the manifold not being welldefined by its dimension, name, and base field. It also needs to be defined by its chart.
Indeed. More precisely, a manifold is defined by its maximal atlas. But there is no way to represent the latter in Sage, nor in any computer system. Accordingly, we cannot check the equality of two manifolds. This is why equality by id seems reasonable here.
I understand that giving two distinct manifolds the same name is bad mathematically, and you are reflecting that in your code. However what if I create a manifold
M
, then decide I want to create a new (and different) manifoldM
of the same dimension over the same base field? When I try to do this naïvely, I get all of the previous chart information much to my surprise (the garbage collector this was more about temporary objects).
You are right: a user may want to restart some computation by creating a manifold with the same name as that used previously. If he is doing so in the same session, the UniqueRepresentation
behavior will return the previously created object. We cannot demand that the user runs _clear_cache_()
before creating the manifold...
comment:33 in reply to: ↑ 32 ; followup: ↓ 34 Changed 5 years ago by
Replying to egourgoulhon:
Replying to tscrim:
It really comes down to what you're doing mostly in the code. If you'd be doing a lot of conversions between points thought of as being in the subset and as in the manifold, then a facade is probably an okay way to go. If you care about associating the point with a particular subset, then it should not be a facade.
I think we are precisely in the first case. In particular we do not care about associating the point with a particular subset (since many overlapping subsets can be introduced that contain the point and none of them is to be privileged with respect to the point). So I would say that we should use the facade.
Then we go with that.
This comes down to the manifold not being welldefined by its dimension, name, and base field. It also needs to be defined by its chart.
Indeed. More precisely, a manifold is defined by its maximal atlas. But there is no way to represent the latter in Sage, nor in any computer system. Accordingly, we cannot check the equality of two manifolds. This is why equality by id seems reasonable here.
Then we are in agreement: equality by id.
I understand that giving two distinct manifolds the same name is bad mathematically, and you are reflecting that in your code. However what if I create a manifold
M
, then decide I want to create a new (and different) manifoldM
of the same dimension over the same base field? When I try to do this naïvely, I get all of the previous chart information much to my surprise (the garbage collector this was more about temporary objects).You are right: a user may want to restart some computation by creating a manifold with the same name as that used previously. If he is doing so in the same session, the
UniqueRepresentation
behavior will return the previously created object. We cannot demand that the user runs_clear_cache_()
before creating the manifold...
As I outlined above, it should be easy enough to remove the UniqueRepresentation
behavior and still retain the desirable behaviors such as hashability.
comment:34 in reply to: ↑ 33 ; followup: ↓ 35 Changed 5 years ago by
Replying to tscrim:
As I outlined above, it should be easy enough to remove the
UniqueRepresentation
behavior and still retain the desirable behaviors such as hashability.
I am on it and should push a new commit soon.
comment:35 in reply to: ↑ 34 Changed 5 years ago by
Replying to egourgoulhon:
Replying to tscrim:
As I outlined above, it should be easy enough to remove the
UniqueRepresentation
behavior and still retain the desirable behaviors such as hashability.I am on it and should push a new commit soon.
Thanks. Also thank you for all your work on this.
comment:36 Changed 5 years ago by
 Commit changed from 6dec6d592a09e56921b9a761827309dd31ae2533 to f342e03e7008831c4789b94b03674c1a0cbbf3a6
comment:37 Changed 5 years ago by
The above commit addresses some of the comments by Travis and Vincent:
 The class
TopManifold
has been renamedTopologicalManifold
and the classTopManifoldSubset
has been renamedTopologicalManifoldSubset
.  A function
Manifold
has been introduced as the unique entry point to construct manifolds (this is the only piece that is imported in the global namespace, as a lazy import).  Tests of
_element_constructor_
are performed by call to the parent.  The classes
TopologicalManifold
,TopologicalManifoldSubset
,Chart
andRealChart
do not longer inherit fromUniqueRepresentation
. Consequently, they have been provided with functions__hash__
,__eq__
,__ne__
and__reduce__
(the latter turned out to be necessary for correct pickling). In addition, the classChart
has been endowed with__getstate__
and__setstate__
.
It seemed necessary to implement a proper __eq__
, i.e. not to have equality by id, since we cannot afford equality by id when relaxing the unique representation, otherwise the pickling test loads(dumps(M)) == M
would failed, since obviously id(loads(dumps(M))
differs from id(M)
.
Other issues mentionned by Travis have not been addressed yet:
 need for an ABC for manifolds and their subsets, introduce a specific class for open subsets (currently they are dealt by class
TopologicalManifold
, with the attributeself._manifold
representing the ambient manifold)  treatment of real and complex fields as base fields (i.e. avoiding to default to the finite precision representations
RR
andCC
)
comment:38 followup: ↓ 39 Changed 5 years ago by
Oh yea...pickling. Uggg...
I worry that the current equality check is too general. Perhaps also for equality add a check to see if the current atlases are equal (or one is a subset of the other), which in turn would have to check equality on charts? Overall it is not pretty, but I would hope one is not checking equality of manifolds too often (and we can somewhat short circuit this by first checking self is other
[which is also a general python recommendation]).
comment:39 in reply to: ↑ 38 Changed 5 years ago by
Replying to tscrim:
Oh yea...pickling. Uggg...
I worry that the current equality check is too general. Perhaps also for equality add a check to see if the current atlases are equal (or one is a subset of the other), which in turn would have to check equality on charts? Overall it is not pretty, but I would hope one is not checking equality of manifolds too often
Unfortunately yes: charts are used as dictionary keys for the coordinate expressions of scalar functions defined on the manifold (these functions will appear massively as the components of tensor fields), therefore one checks often equality of charts and the latter starts by checking the equality of manifolds on which they are defined. By the way, if we introduce the check of atlases in the manifold equality, there is a danger of endless loop: to compare charts, one first compare their domains, i.e. manifolds... To circumvent this, maybe we should introduce two comparison operators: a fast __eq__
, which cheks only the name, dimension and base field and a more mathematically meaningful is_isomorphic_to
, which checks in addition the atlases.
comment:40 followup: ↓ 41 Changed 5 years ago by
I've already noticed some important loss of performance due to the equality not being by id; I am afraid that for tensor computations this will become much worse. Moreover, a sophisticated equality check (either with __eq__
or is_isomorphic_to
) seems difficult to acheive: checking the equality of the userdefined atlases is definitevely not sufficient to assert the mathematical equality of two manifolds; one should compare the maximal atlases instead, which is impossible. For example, if one first construct S^{2} with an atlas of two stereographic charts and then another S^{2} with an atlas of two polar charts, the two atlases differ, while both objects represent the same manifold. There is also the issue of endless loop mentioned in comment:39.
For the above reasons, I am considering to revert to the UniqueRepresentation
for manifolds. To solve the issue of the redefinition by the end user disccused in comment:32, we could have some handling of the cache in the function Manifold
, so that
M1 = Manifold(2, 'M') M2 = Manifold(2, 'M')
will construct two different objects. What do you think?
comment:41 in reply to: ↑ 40 ; followup: ↓ 43 Changed 5 years ago by
Replying to egourgoulhon:
I've already noticed some important loss of performance due to the equality not being by id; I am afraid that for tensor computations this will become much worse.
Did you have as the first lines of the __eq__
being
if self is other: return True
since "most" equality checks should be by identity. Granted, we do lose some speed because this is a python check, as opposed to a cython check.
Moreover, a sophisticated equality check (either with
__eq__
oris_isomorphic_to
) seems difficult to acheive: checking the equality of the userdefined atlases is definitevely not sufficient to assert the mathematical equality of two manifolds; one should compare the maximal atlases instead, which is impossible. For example, if one first construct S^{2} with an atlas of two stereographic charts and then another S^{2} with an atlas of two polar charts, the two atlases differ, while both objects represent the same manifold.
__eq__
does not have to represent mathematical equality; IIRC we already have examples of this in Sage, both when __eq__
is weaker and stronger than mathematical equality (which often times people wish it could be isomorphism). So we are okay with making __eq__
not reflect the mathematical equality.
There is also the issue of endless loop mentioned in comment:39.
The design reflects the mathematics as charts are morphism from R^{n} > M and that the manifold M is an abstract set of points. My proposal goes around the mathematical definition of a manifold as a set of points by saying the manifold M is defined by its charts. So this approach is definitely bad.
For the above reasons, I am considering to revert to the
UniqueRepresentation
for manifolds. To solve the issue of the redefinition by the end user disccused in comment:32, we could have some handling of the cache in the functionManifold
, so thatM1 = Manifold(2, 'M') M2 = Manifold(2, 'M')will construct two different objects. What do you think?
I worry that this will cause subtle issues with pickling. For example, if we clear the cache when creating M2
, then loads(dumps(M1))
will probably not equal M1
, but instead equal M2
. I haven't checked if this actually happens.
There are a few different options that I see at this point.
 Drop pickling
==
support and usesage.misc.fast_methods.WithEqualityById
(this will get your speed back). However we could get close by implementing pickling as saving the current atlas.  Go back to
UniqueRepresentation
, but also have it additionally keyed by the time it was created:sage: import time sage: time.time() 1446566813.121567
You could do this by having@staticmethod def __classcall__(cls, dim, name, latex, R, time_key=None): if time_key is None: from time import time time_key = time() return super(TopologicalManifold, cls).__classcall__(cls, dim, latex, R, time_key=time_key) def __init__(self, dim, name, latex, R, time_key): # Just ignore the time_key input
(or doing it via aUniqueFactory
).  Warn the user that they can only create one manifold of a given (latex) name of a given dimension over a given field.
 Figure out some other way to better uniquely specify a manifold.
 Ask someone else, like Jeroen, Simon, Volker, and/or sagedevel, for other options.
Personally I would go for first one since pickling within a session is, I believe, usually not needed/used.
comment:42 Changed 5 years ago by
 Commit changed from f342e03e7008831c4789b94b03674c1a0cbbf3a6 to 902908b41a95d3455bfcc497997ad2054c530a96
Branch pushed to git repo; I updated commit sha1. New commits:
902908b  Revert to UniqueRepresentation for topological manifolds and charts, with the possibility to reuse manifold names.

comment:43 in reply to: ↑ 41 Changed 5 years ago by
Replying to tscrim:
Replying to egourgoulhon:
I've already noticed some important loss of performance due to the equality not being by id; I am afraid that for tensor computations this will become much worse.
Did you have as the first lines of the
__eq__
beingif self is other: return True
Actually no (except for points).
since "most" equality checks should be by identity.
Well, as soon as we have more than two charts on a manifold, this is no longer true.
Granted, we do lose some speed because this is a python check, as opposed to a cython check.
__eq__
does not have to represent mathematical equality; IIRC we already have examples of this in Sage, both when__eq__
is weaker and stronger than mathematical equality (which often times people wish it could be isomorphism). So we are okay with making__eq__
not reflect the mathematical equality.
OK.
For the above reasons, I am considering to revert to the
UniqueRepresentation
for manifolds. To solve the issue of the redefinition by the end user disccused in comment:32, we could have some handling of the cache in the functionManifold
, so thatM1 = Manifold(2, 'M') M2 = Manifold(2, 'M')will construct two different objects. What do you think?
I worry that this will cause subtle issues with pickling. For example, if we clear the cache when creating
M2
, thenloads(dumps(M1))
will probably not equalM1
, but instead equalM2
. I haven't checked if this actually happens.
Thanks for pointing this, I think you are right.
There are a few different options that I see at this point.
 Drop pickling
==
support and usesage.misc.fast_methods.WithEqualityById
(this will get your speed back). However we could get close by implementing pickling as saving the current atlas.
I am quite reluctant to drop pickling ==
support, although, as you say, pickling within a session is usually not needed/used. This would break all TestSuite()
tests, except of course if we run them with skip='_test_pickling'
.
 Go back to
UniqueRepresentation
, but also have it additionally keyed by the time it was created:sage: import time sage: time.time() 1446566813.121567You could do this by having@staticmethod def __classcall__(cls, dim, name, latex, R, time_key=None): if time_key is None: from time import time time_key = time() return super(TopologicalManifold, cls).__classcall__(cls, dim, latex, R, time_key=time_key) def __init__(self, dim, name, latex, R, time_key): # Just ignore the time_key input(or doing it via aUniqueFactory
).
I like very much this one; thanks a lot for suggesting it! I have implemented it in the above commit, with a small modification: I have not redefined TopologicalManifold.__classcall__
(which is inherited from UniqueRepresentation
), instead I have simply set the time tag in the function Manifold
. Everything works well: see the rubric "Reusability of the manifold name" in the documentation of the function Manifold
.
 Ask someone else, like Jeroen, Simon, Volker, and/or sagedevel, for other options.
Good idea, I will.
comment:44 Changed 5 years ago by
 Description modified (diff)
The phrase "manifold over a field K" sounds odd to me. Is it used in the literature? What if K is a finite field? It seems that if X is a finite discrete space, for every finite field F and for every nonnegative integer n, then X is a manifold over F of dimension n: F and n play no role. (I'm assuming that finite fields have been given the discrete topology.)
I think you might say "topological manifold over a topological field K", since obviously the topology on K is critical. Or you could omit "over a field K", and mention in the documentation that users can specify a topological field (like \CC, rather than the default \RR) if they want.