Opened 4 years ago
Closed 3 years ago
#18529 closed enhancement (fixed)
Topological manifolds: basics
Reported by:  egourgoulhon  Owned by:  egourgoulhon 

Priority:  major  Milestone:  sage7.1 
Component:  geometry  Keywords:  topological manifolds 
Cc:  mmancini  Merged in:  
Authors:  Eric Gourgoulhon, Travis Scrimshaw  Reviewers:  Travis Scrimshaw, Eric Gourgoulhon 
Report Upstream:  N/A  Work issues:  
Branch:  00d265c (Commits)  Commit:  00d265cf2855121dba914868264da6ea3a9c42af 
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:
ManifoldSubset
: generic subset of a topological manifold (the open subsets being implemented by the subsclassTopologicalManifold
)TopologicalManifold
: topological manifold over a topological field K
ManifoldPoint
: point in 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
as well as the singleton classesTopologicalStructure
and RealTopologicalStructure
.
TopologicalManifold
is intended to serve as a base class for specific manifolds, like smooth manifolds (K=R) and complex manifolds (K=C). The followup ticket, implementing continuous functions to the base field, is #18640.
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 (126)
comment:1 Changed 4 years ago by
 Description modified (diff)
comment:2 followup: ↓ 4 Changed 4 years ago by
comment:3 Changed 4 years ago by
 Description modified (diff)
comment:4 in reply to: ↑ 2 Changed 4 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 4 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 4 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 4 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 4 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 4 years ago by
 Description modified (diff)
comment:10 Changed 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 followups: ↓ 28 ↓ 53 Changed 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 years ago by
 Commit changed from 6dec6d592a09e56921b9a761827309dd31ae2533 to f342e03e7008831c4789b94b03674c1a0cbbf3a6
comment:37 followup: ↓ 45 Changed 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 years ago by
 Description modified (diff)
comment:45 in reply to: ↑ 37 ; followup: ↓ 46 Changed 4 years ago by
Replying to egourgoulhon:
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 testloads(dumps(M)) == M
would failed, since obviouslyid(loads(dumps(M))
differs fromid(M)
.
Can't you just drop that test? I don't think it's something that has to be true:
sage: A=object() sage: loads(dumps(A)) == A False
comment:46 in reply to: ↑ 45 Changed 4 years ago by
Replying to nbruin:
Replying to egourgoulhon:
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 testloads(dumps(M)) == M
would failed, since obviouslyid(loads(dumps(M))
differs fromid(M)
.Can't you just drop that test? I don't think it's something that has to be true:
Well actually this is part of the _test_pickling
method, as defined in sage/structure/sage_object.pyx
. So shall we run the test suite as TestSuite(M).run(skip='_test_pickling')
or shall we redefine _test_pickling
?
comment:47 followup: ↓ 49 Changed 4 years ago by
There is no reason to write ugly stuff like
self.__eq__(other)
and
foo.__hash__()
The alternatives
self == other
and
hash(foo)
are easier to read and faster.
comment:48 followup: ↓ 50 Changed 4 years ago by
In think this also holds for type(foo)
instead if foo.__class__
but I'm not 100% sure if those are really equivalent.
comment:49 in reply to: ↑ 47 Changed 4 years ago by
Replying to jdemeyer:
There is no reason to write ugly stuff like
self.__eq__(other)and
foo.__hash__()The alternatives
self == otherand
hash(foo)are easier to read and faster.
OK I will change this. Thanks.
comment:50 in reply to: ↑ 48 Changed 4 years ago by
Replying to jdemeyer:
In think this also holds for
type(foo)
instead iffoo.__class__
but I'm not 100% sure if those are really equivalent.
From this link and that one, it seems that for Python newstyle classes (i.e. the only classes used in Sage), type(foo)
is fully equivalent to foo.__class__
and it is indeed best practice to use type(foo)
.
comment:51 Changed 4 years ago by
 Commit changed from 902908b41a95d3455bfcc497997ad2054c530a96 to 252e616cc053a3b76ee563282222507cd78c9fb8
Branch pushed to git repo; I updated commit sha1. New commits:
252e616  Remove UniqueRepresentation, leaving only WithEqualityById, for topological manifolds and charts.

comment:52 Changed 4 years ago by
 Description modified (diff)
The above commit takes into account this discussion on sagedevel, as well as the first recommendation of Travis in comment:41 : it removes UniqueRepresentation
for manifolds and charts, leaving only WithEqualityById
. Some methods _test_pickling
have been introduced. They are weaker than SageObject._test_pickling
in the sense that they do not demand loads(dumps(M)) == M
(which equalitybyid forbids without any unique representation). However, these local _test_pickling
methods perform non trivial tests: they guarantee that loads(dumps(M))
proceeds without any error and they check the identity of some characteristics between the unpickled object and the original one. All the test suites are passed.
In addition the above commit takes into account the recommendation of Jeroen in comment:49.
comment:53 in reply to: ↑ 27 Changed 4 years ago by
Replying to tscrim:
 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.
Actually, the precision is not used in the current setting, so neither RR
nor RealField
is really necessary. RR
was used as a substitute for the true real field, which cannot be represented in the computer. Looking further, why not using Sage's RealLazyField
for this? It seems that it is intended to be closer to the true R. One drawback is that we have (for the moment)
sage: RLF in Fields().Topological() False
So RLF
cannot be passed to the manifold constructor.
The same considerations apply of course to ComplexLazyField
.
comment:54 followup: ↓ 55 Changed 4 years ago by
What we need is a category or ABC of realizations of the real field that allows us to set things like the category across the board and give us ways to tell a manifold is over the real numbers. Although perhaps in this case we can just handle the strings "real"
and "complex"
and have them default to the category of Manifolds(RR)
and Manifolds(CC)
.
comment:55 in reply to: ↑ 54 ; followup: ↓ 56 Changed 4 years ago by
Replying to tscrim:
What we need is a category or ABC of realizations of the real field that allows us to set things like the category across the board and give us ways to tell a manifold is over the real numbers. Although perhaps in this case we can just handle the strings
"real"
and"complex"
and have them default to the category ofManifolds(RR)
andManifolds(CC)
.
Yes, for the time being, we could have two attributes in the manifold class:
_field_type
, a string with values'real'
,'complex'
or'other'
, which is the thing that is checked against to know if we are dealing with e.g. a real manifold; for instance, to decide whether the charts are constructed in the subclassRealChart
of the generic classChart
._field
, which contains the field for the manifold category; it could default toRR
for_field_type == 'real'
.
Do you agree?
comment:56 in reply to: ↑ 55 Changed 4 years ago by
Replying to egourgoulhon:
Replying to tscrim:
What we need is a category or ABC of realizations of the real field that allows us to set things like the category across the board and give us ways to tell a manifold is over the real numbers. Although perhaps in this case we can just handle the strings
"real"
and"complex"
and have them default to the category ofManifolds(RR)
andManifolds(CC)
.Yes, for the time being, we could have two attributes in the manifold class:
_field_type
, a string with values'real'
,'complex'
or'other'
, which is the thing that is checked against to know if we are dealing with e.g. a real manifold; for instance, to decide whether the charts are constructed in the subclassRealChart
of the generic classChart
._field
, which contains the field for the manifold category; it could default toRR
for_field_type == 'real'
Do you agree?
Yes that sounds like a good plan along with some simple type checking for default values of _field_type
against RealField
and ComplexField
since (IMO) these are what most users will use for the reals and complexes.
comment:57 Changed 4 years ago by
 Commit changed from 252e616cc053a3b76ee563282222507cd78c9fb8 to 65186990b1106f89652107356b60faa048113915
Branch pushed to git repo; I updated commit sha1. New commits:
6518699  Introduce the attribute _field_type in class TopologicalManifold to check for real and complex manifolds.

comment:58 Changed 4 years ago by
 Commit changed from 65186990b1106f89652107356b60faa048113915 to 0b08b114e03c063bd2e500ac900fd843fb12673b
comment:59 followup: ↓ 61 Changed 4 years ago by
I've done some small doc/review tweaks, but I was wondering if you were doing any changes to it, because I want to do some moderate refactoring to try and simplify the structure. In particular, you are not really using the subsets as facade parents as the points know what subset they belong to.
comment:60 Changed 4 years ago by
 Reviewers set to Travis Scrimshaw
comment:61 in reply to: ↑ 59 ; followups: ↓ 62 ↓ 74 Changed 4 years ago by
Replying to tscrim:
I've done some small doc/review tweaks,
Thanks for these changes!
but I was wondering if you were doing any changes to it,
No, not at the moment: I've worked on the subsequent tickets to check if the introduced changes (in particular the removal of unique representation) propagate smoothly. So far, so good...
because I want to do some moderate refactoring to try and simplify the structure. In particular, you are not really using the subsets as facade parents as the points know what subset they belong to.
Well, it is true that points have an attribute called _subset
, but this is a misnomer: _creation_subset
would have been better. Indeed, from a pure mathematical point of view, a point of a manifold has no privileged subset attached to it. On the contrary, the point belongs to an infinite number of intersecting subsets. The attribute _subset
, which is set to the facade parent at the point creation, is used only for fast check in the methods TopologicalManifoldSubset.__contains__
and TopologicalManifold.__contains__
. I think it can be suppressed, at the price of a small decrease in efficiency. Therefore, I still think that the facade mechanism is appropriate here: the creation subset (i.e. the facade parent) should not play any role: only the whole manifold matters. This is particularly true when dealing with tangent planes (ticket #19092): for a given point p, we do not want to have two tangent spaces, T_p M and T_p U with U open subset of M, do we?
comment:62 in reply to: ↑ 61 ; followup: ↓ 63 Changed 4 years ago by
Replying to egourgoulhon:
Replying to tscrim:
but I was wondering if you were doing any changes to it,
No, not at the moment: I've worked on the subsequent tickets to check if the introduced changes (in particular the removal of unique representation) propagate smoothly. So far, so good...
I will then start my refactoring. I will try to make every change as granular as possible in the commits so we can cherrypick changes if you don't necessarily agree with them.
because I want to do some moderate refactoring to try and simplify the structure. In particular, you are not really using the subsets as facade parents as the points know what subset they belong to.
Well, it is true that points have an attribute called
_subset
, but this is a misnomer:_creation_subset
would have been better. Indeed, from a pure mathematical point of view, a point of a manifold has no privileged subset attached to it. On the contrary, the point belongs to an infinite number of intersecting subsets. The attribute_subset
, which is set to the facade parent at the point creation, is used only for fast check in the methodsTopologicalManifoldSubset.__contains__
andTopologicalManifold.__contains__
. I think it can be suppressed, at the price of a small decrease in efficiency. Therefore, I still think that the facade mechanism is appropriate here: the creation subset (i.e. the facade parent) should not play any role: only the whole manifold matters. This is particularly true when dealing with tangent planes (ticket #19092): for a given point p, we do not want to have two tangent spaces, T_p M and T_p U with U open subset of M, do we?
At present, anytime an element is passed to the _element_constructor_
, a new instance of that point is created. In a way, you are hacking the coercion/category framework by setting the parent of the point to be the manifold, so M(p)
just shortcuts out to return p
. The code tells me that points should be elements of a particular subset.
For the tangent spaces, you should coerce the point to the manifold in the distinguished chart as the input. You're going to have to do this anyways:
sage: M = Manifold(2, 'M', field='real', type='topological') sage: X.<x,y> = M.chart() sage: U.<u,v> = M.chart() sage: trans = X.transition_map(U, [x2,y2]) sage: p = M.point((0,0), X) sage: p2 = M.point((2,2), U) sage: p == p2 True sage: p is p2 False
How about this, let me make my changes and you can see what breaks or how much things slow down and we will use that to decide what we should do going forward. Does that sound reasonable to try? (In general, I would also argue that users should create their points from the manifold using one of those charts.)
comment:63 in reply to: ↑ 62 ; followup: ↓ 64 Changed 4 years ago by
Replying to tscrim:
I will then start my refactoring. I will try to make every change as granular as possible in the commits so we can cherrypick changes if you don't necessarily agree with them.
OK very good. Have you noticed that some doctests failed in the latest commit? This due to a typo: "coordintes" instead of "coordinates" in the replacement of
if self._restrictions != []: substitutions = dict(zip(self._xx, coordinates))
by
if self._restrictions: substitutions = {x: coordintes[i] for i,x in enumerate(self._xx)}
in chart.py
. By the way, why is the second form better than the first one?
How about this, let me make my changes and you can see what breaks or how much things slow down and we will use that to decide what we should do going forward. Does that sound reasonable to try?
Yes, absolutely!
comment:64 in reply to: ↑ 63 Changed 4 years ago by
Replying to egourgoulhon:
Have you noticed that some doctests failed in the latest commit? This due to a typo: "coordintes" instead of "coordinates" in the replacement of
if self._restrictions != []: substitutions = dict(zip(self._xx, coordinates))by
if self._restrictions: substitutions = {x: coordintes[i] for i,x in enumerate(self._xx)}in
chart.py
. By the way, why is the second form better than the first one?
No, but I didn't run all of the doctests before I pushed (in part because I was planning to do a lot more work).
I think the latter is more clear in terms of code. I also thought I had run some timings before that the second was faster, but this is not the case:
sage: k,v = range(1000), range(1000) sage: %timeit d = dict(zip(k,v)) The slowest run took 5.50 times longer than the fastest. This could mean that an intermediate result is being cached 10000 loops, best of 3: 58.4 µs per loop sage: %timeit d = {x: v[i] for i,x in enumerate(k)}
So I will just revert this change (and any others like it).
comment:65 followup: ↓ 66 Changed 4 years ago by
Status report: Still working on refactoring. I have had to go through a few iterations, but I think I've arrived to a model which will scale nicely with the additional manifold structures of smooth/differentiable/(almost)complex. I'm hoping to get it posted in a day or two.
Question, do you know what are the main bottleneck operations are? I'm wondering if there would be any benefits from doing a partial cythonization while I'm mucking around. If you don't know offhand, then I wouldn't worry about it.
comment:66 in reply to: ↑ 65 Changed 4 years ago by
Replying to tscrim:
Status report: Still working on refactoring. I have had to go through a few iterations, but I think I've arrived to a model which will scale nicely with the additional manifold structures of smooth/differentiable/(almost)complex. I'm hoping to get it posted in a day or two.
OK very good. Thank you for working on this!
Question, do you know what are the main bottleneck operations are? I'm wondering if there would be any benefits from doing a partial cythonization while I'm mucking around.
In actual calculations, the main bottleneck is the simplification of symbolic expressions, which is performed in Lisp by Maxima. So I don't think that cythonizing would improve much here. On the other hand, parallelization over the components of vector fields (and more generally tensorial objects), as performed in #18100, helps a lot.
comment:67 followup: ↓ 68 Changed 4 years ago by
 Branch changed from public/manifolds/top_manif_basics to u/tscrim/top_manifolds_refactor
 Commit changed from 0b08b114e03c063bd2e500ac900fd843fb12673b to 0fb39df7fafe7f0a765bf73b3f34a6cb41e65c40
 Status changed from needs_review to needs_info
Okay, I've refactored the code and it works up to "trivial" doctest failures. So almost all functionality on the outside hasn't changed, but internally, a lot of the logic within each function has changed at the cost of a slightly more complex class hierarchy (there might even be some more places to streamline things around too). Something else to do would be to define coercions between the respective subsets, but that we can do on a follow up since these are just sets at this point.
The biggest thing to note is that I have separated out the structure of the manifold into a separate class. This has several distinct advantages:
 It has better encapsulation of data, which should result in fewer copies of the defining data needed and fewer duplication of functions.
 It could allow us to strength/weaken the structure on the manifold dynamically.
 We might only need to have one manifold class and one subset class, that way we don't have to duplicate documentation.
The main drawback I see at the point is we don't expose the attributes of the structure (e.g., the differential order) directly from the manifold instance. However, if we want this behavior, then we can attach the appropriate data as (hidden) (class) attributes to keep some of the modularity; it just results in more classes.
I didn't want to change documentation until I knew you approved of this refactoring (or the alternative proposed above). Please tell me what you think and how you feel it fits with the differentiable manifolds part. I hope this does not cause too much trouble with rebasing.
New commits:
0fb39df  Refactoring the code to separate out the structural part of the manifold.

comment:68 in reply to: ↑ 67 ; followup: ↓ 69 Changed 4 years ago by
The refactoring with the new classes AbstractObject
, AbstractSet
, ManifoldSubset
and TopologicalSubmanifold
looks good. It clarify things, thanks! A suggestion regarding the naming: what about replacing AbstractObject
, which sounds too general, by AbstractNamedObject
, which would better reflect the class content?
Regarding the separation manifold/structure, I am wondering how this could fit with differentiable manifolds? Since the structure classes are singleton, they store things like the differential order but not properties specific to a given manifold, like the set of vector frames defined on the manifold or its module of vector fields. Then, how could one have a single class for all kind of manifolds? For instance, the class DifferentiableManifold
introduced in #18783 inherits from TopologicalManifold
and has the additional attributes _frames
, _coframes
, _frame_changes
, _parallelizable_parts
, _vector_field_modules
, etc., which have no meaning for a topological manifold. Besides, we don't want the user to write something like v = M.structure().vector_frame(...)
instead of v = M.vector_frame(...)
, do we?
comment:69 in reply to: ↑ 68 ; followup: ↓ 70 Changed 4 years ago by
Replying to egourgoulhon:
The refactoring with the new classes
AbstractObject
,AbstractSet
,ManifoldSubset
andTopologicalSubmanifold
looks good. It clarify things, thanks! A suggestion regarding the naming: what about replacingAbstractObject
, which sounds too general, byAbstractNamedObject
, which would better reflect the class content?
That is okay with me.
Regarding the separation manifold/structure, I am wondering how this could fit with differentiable manifolds? Since the structure classes are singleton, they store things like the differential order but not properties specific to a given manifold, like the set of vector frames defined on the manifold or its module of vector fields.
I wasn't imagining that they would be singletons, as they would carry instance information like differential order.
Then, how could one have a single class for all kind of manifolds? For instance, the class
DifferentiableManifold
introduced in #18783 inherits fromTopologicalManifold
and has the additional attributes_frames
,_coframes
,_frame_changes
,_parallelizable_parts
,_vector_field_modules
, etc., which have no meaning for a topological manifold. Besides, we don't want the user to write something likev = M.structure().vector_frame(...)
instead ofv = M.vector_frame(...)
, do we?
Good point. Then rather these being instance objects, they should probably be mixin classes. This would help keep some separation of concerns, avoid some redundancies, and make it easy to manage the distinction between the full manifold and the submanifolds.
comment:70 in reply to: ↑ 69 ; followup: ↓ 72 Changed 4 years ago by
Replying to tscrim:
I wasn't imagining that they would be singletons,
OK (I thought that since you were using singletons for the topological structure, you planed to use them for any kind of structure).
as they would carry instance information like differential order.
Regarding the specific case of the differential order, we may also consider that it is part of the structure per se and not of the instance, i.e. consider that a C^{2}structure differs from a C^{4}structure.
Then, how could one have a single class for all kind of manifolds? For instance, the class
DifferentiableManifold
introduced in #18783 inherits fromTopologicalManifold
and has the additional attributes_frames
,_coframes
,_frame_changes
,_parallelizable_parts
,_vector_field_modules
, etc., which have no meaning for a topological manifold. Besides, we don't want the user to write something likev = M.structure().vector_frame(...)
instead ofv = M.vector_frame(...)
, do we?Good point. Then rather these being instance objects, they should probably be mixin classes. This would help keep some separation of concerns, avoid some redundancies, and make it easy to manage the distinction between the full manifold and the submanifolds.
Could you please describe further how you would use mixin classes? and why this would be superior to the simple heritage TopologicalManifold < DifferentiableManifold
?
comment:71 Changed 4 years ago by
comment:72 in reply to: ↑ 70 ; followup: ↓ 73 Changed 4 years ago by
Replying to egourgoulhon:
Could you please describe further how you would use mixin classes? and why this would be superior to the simple heritage
TopologicalManifold < DifferentiableManifold
?
If we just had a simple inheritance, then we'd have this as our hierarchy:
Abstract / \ Subset TopManifold  ____/   /  TopSub DiffManifold  ____/  / DiffSub
However, with a mixin, we would have this:
Abstract / \ Subset TopManifold  ____/   /  TopSub DiffMixin   / \  DiffSub DiffManifold
In particular, notice that this does not introduce another diamond problem. It also makes it easier to add another class at the Subset
and TopManifold
level if we ever wanted to.
Ideally, I would like to abstract away the Subset
parts to a mixin to completely avoid any diamonds, but I couldn't really figure out a good way to make that work.
comment:73 in reply to: ↑ 72 Changed 4 years ago by
Thanks for these explanations.
comment:74 in reply to: ↑ 61 Changed 4 years ago by
Replying to egourgoulhon:
No, not at the moment: I've worked on the subsequent tickets to check if the introduced changes (in particular the removal of unique representation) propagate smoothly. So far, so good...
Bad news: the removal of unique representation breaks parallel computations in #19147 (affine connections). For instance, in the branch of #19147, if one performs
sage: M = Manifold(3, 'M') sage: X.<x,y,z> = M.chart() sage: nab = M.affine_connection('nabla', r'\nabla') sage: nab[0,0,1], nab[2,1,2] = x^2, y*z sage: use_multiproc(2) # parallelization on 2 proc sage: nab.riemann()
one gets the error message:
RuntimeError: There is a bug in the coercion code in Sage. Both x (=Scalar field on the 3dimensional differentiable manifold M) and y (=Scalar field on the 3dimensional differentiable manifold M) are supposed to have identical parents but they don't. In fact, x has parent 'Algebra of differentiable scalar fields on the 3dimensional differentiable manifold M' whereas y has parent 'Algebra of differentiable scalar fields on the 3dimensional differentiable manifold M'
and sage terminates badly (core dumped).
The reason is that the parallelization is using the pickling: the parallel iterator sage.parallel.multiprocessing_sage.parallel_iter
invokes the function sage.misc.fpickle.pickle_function
.
With unique representation, the pickling was fine and the above issue did not occur.
Note that parallelization of heavy computations like that of the Riemann tensor is really important!...
comment:75 followup: ↓ 76 Changed 4 years ago by
I think what we will have to do is one (or more) of the following:
 implement custom multiprocessing,
 move the parallelization code into the scalar fields algebra,
 implement a custom coercion scheme by overriding
__add__
, etc., or  improve coercion support for non
UniqueRepresentation
objects.
I think the first or second option will be the best possibility for this to work.
comment:76 in reply to: ↑ 75 ; followup: ↓ 77 Changed 4 years ago by
 Cc mmancini added
Replying to tscrim:
I think what we will have to do is one (or more) of the following:
 implement custom multiprocessing,
 move the parallelization code into the scalar fields algebra,
 implement a custom coercion scheme by overriding
__add__
, etc., or improve coercion support for non
UniqueRepresentation
objects.I think the first or second option will be the best possibility for this to work.
The first looks to me much better than the second one: the nice feature of the parallelization framework as implemented in #18100 is that it is implemented at the Components
level, i.e. for any indexed set of ring elements, whatever the ring. It sounds bad to redefine this for the special case where the ring is an algebra of scalar fields. I shall discuss with Marco about implementing some multiprocessing that does not rely on pickling.
Another solution would be to revert to UniqueRepresentation
for manifolds (once again!). Since now the user creates manifolds via the frontend function Manifold
and not by a direct call to the manifold class constructor, the main issue raised at the end of comment:31 could be overcome by the handling of the cache in Manifold
. In this way
sage: M1 = Manifold(2, 'M') sage: M2 = Manifold(2, 'M')
would result in two different objects, as desired, but
sage: M1 == loads(dumps(M1))
would still work, because the unpickling does not go through the function Manifold
.
There remains the issue with UniqueRepresentation
raised by Nils Bruin in the sagedevel thread: Avoid UniqueRepresentation if you can. It requires expensive processing of construction parameters and hence introduces bad overhead and it introduces "global variables" in a way that is much worse than global variables.
However, in a typical work session, one should not create so many manifolds, so the "expensive processing of construction parameters" is not a too severe issue.
comment:77 in reply to: ↑ 76 ; followup: ↓ 78 Changed 4 years ago by
Replying to egourgoulhon:
Another solution would be to revert to
UniqueRepresentation
for manifolds (once again!). Since now the user creates manifolds via the frontend functionManifold
and not by a direct call to the manifold class constructor, the main issue raised at the end of comment:31 could be overcome by the handling of the cache inManifold
. In this waysage: M1 = Manifold(2, 'M') sage: M2 = Manifold(2, 'M')would result in two different objects, as desired, but
sage: M1 == loads(dumps(M1))would still work, because the unpickling does not go through the function
Manifold
.
This breaks pickling:
sage: class Foo(UniqueRepresentation): ....: def __init__(self, i): ....: pass ....: sage: F1 = Foo(1) sage: Foo._clear_cache_() sage: F2 = Foo(1) sage: F1 is F2 False sage: F1 == loads(dumps(F1)) False sage: F2 == loads(dumps(F1)) True
The reason is pickling still goes through the UniqueRepresentation
cache. I am still convinced we should not be using UniqueRepresentation
for the manifolds.
comment:78 in reply to: ↑ 77 ; followup: ↓ 79 Changed 4 years ago by
Replying to tscrim:
This breaks pickling:
Yes but by "handling of the cache" I had in mind not a call to _clear_cache_
but rather adding a
unique tag to the constructor, such as the time tag that you already suggested. In this way, pickling would work. But I agree that this is a trick and that manifolds do not have a unique (mathematical) representation. However, given that in general we cannot decide whether two given manifolds are equal (isomorphic), making each of them unique with some tag could be an idea... What would be the drawbacks?
comment:79 in reply to: ↑ 78 Changed 4 years ago by
Marco and I have discussed the thing this morning. It's clear that the multiprocessing
Python module relies on pickling; some references are
 https://docs.python.org/2/library/multiprocessing.html
 http://matthewrocklin.com/blog/work/2013/12/05/ParallelismandSerialization/
 http://stackoverflow.com/questions/8804830/pythonmultiprocessingpicklingerror
Therefore, unless one develops a brand new way of parallelizing Python codes, good pickling is required for parallelization. Given the importance of parallelization in tensor calculus, I am really considering reverting to UniqueRepresentation
for the manifold classes. The argument goes as follows: from the sagedevel discussion, we get that manifolds must implement equality by identity (i.e. inherit from WithEqualityById
), mostly because there exist no algorithm to decide whether two given manifolds are homeo/diffeomorphic (especially with manifolds with incomplete atlases, as we manipulate them). Then to have good pickling, i.e. to ensure M == loads(dumps(M))
, the only way is to inherit from CachedRepresentation
as well. Having both WithEqualityById
and CachedRepresentation
impliesUniqueRepresentation
.
comment:80 followup: ↓ 81 Changed 4 years ago by
I did some exploring in the coercion code and some testing using this:
from sage.structure.parent import Parent from sage.structure.element_wrapper import ElementWrapper from sage.misc.fast_methods import WithEqualityById from sage.categories.additive_groups import AdditiveGroups class Foo(Parent, WithEqualityById): def __init__(self): Parent.__init__(self, category=AdditiveGroups()) def __reduce__(self): return (Foo, ()) def _coerce_map_from_(self, R): return isinstance(R, Foo) def _element_constructor_(self, x): if isinstance(x, ElementWrapper) and isinstance(x.parent(), Foo): return self(x.value) return super(Foo, self)._element_constructor_(x) class Element(ElementWrapper): def _add_(self, other): return self.parent()(self.value+other.value)
which I think is a minimal example of the behavior you want. Pickling and coercion seem to work fine:
sage: F = Foo() sage: F2 = loads(dumps(F)) sage: F(1) + F2(1) 2 sage: F == F2 False
Furthermore, from looking at your code, the reason why it now breaks is you're assuming unique representation by not passing the scalar field algebra as the first argument of the scalar field. So it's getting mismatches because the domain.scalar_field_algebra()
may not be the scalar field algebra that was suppose to be creating that particular scalar field. So for the scalar fields, I would instead have the first argument be the corresponding algebra and get the domain from that (with appropriate handling of parameters for the methods which create the scalar fields). You might also consider caching the scalar field algebra that gets created for a particular manifold to have unique representation behavior.
comment:81 in reply to: ↑ 80 ; followup: ↓ 82 Changed 4 years ago by
Replying to tscrim:
I did some exploring in the coercion code and some testing using this:
Thanks for the exploration. The trick
def _coerce_map_from_(self, R): return isinstance(R, Foo)
is too permissive to be applicable to scalar field algebras. For them, the coercion is currently based on the concept of restriction to a subdomain, i.e. there is a coerce map from C^{k}(M) to C^{k}(N) iff N is a subset of M (cf. the code of _coerce_map_from_
in scalar_field_algebra.py
). It is very desirable to keep this feature, which is mathematically neat.
Furthermore, from looking at your code, the reason why it now breaks is you're assuming unique representation by not passing the scalar field algebra as the first argument of the scalar field. So it's getting mismatches because the
domain.scalar_field_algebra()
may not be the scalar field algebra that was suppose to be creating that particular scalar field. So for the scalar fields, I would instead have the first argument be the corresponding algebra and get the domain from that (with appropriate handling of parameters for the methods which create the scalar fields).
I've done this (actually this should have been done before: it is cleaner to have the parent as first argument when constructing elements). But this does not solve the problem. There is a difference though: on the example of comment:74, we do no longer get the coercion error message but the ECL error message arising from Maxima; the latter was previously appearing after the coercion error message:
;;; ;;; Stack overflow. ;;; Jumping to the outermost toplevel prompt ;;;
and that a lot of text until Abandon (core dumped)
. I've pushed the new code (i.e. with the algebra as first argument of scalar fields) to the branch of #19147, in case you want to have a look.
You might also consider caching the scalar field algebra that gets created for a particular manifold to have unique representation behavior.
This was already the case (cf. the code of DifferentiableManifold.scalar_field_algebra()
).
comment:82 in reply to: ↑ 81 ; followup: ↓ 84 Changed 4 years ago by
Replying to egourgoulhon:
Replying to tscrim:
The trick
def _coerce_map_from_(self, R): return isinstance(R, Foo)is too permissive to be applicable to scalar field algebras. For them, the coercion is currently based on the concept of restriction to a subdomain, i.e. there is a coerce map from C^{k}(M) to C^{k}(N) iff N is a subset of M (cf. the code of
_coerce_map_from_
inscalar_field_algebra.py
). It is very desirable to keep this feature, which is mathematically neat.
I completely agree. I was just using it for illustrative purposes.
Furthermore, from looking at your code, the reason why it now breaks is you're assuming unique representation by not passing the scalar field algebra as the first argument of the scalar field. So it's getting mismatches because the
domain.scalar_field_algebra()
may not be the scalar field algebra that was suppose to be creating that particular scalar field. So for the scalar fields, I would instead have the first argument be the corresponding algebra and get the domain from that (with appropriate handling of parameters for the methods which create the scalar fields).I've done this (actually this should have been done before: it is cleaner to have the parent as first argument when constructing elements). But this does not solve the problem. There is a difference though: on the example of comment:74, we do no longer get the coercion error message but the ECL error message arising from Maxima; the latter was previously appearing after the coercion error message:
;;; ;;; Stack overflow. ;;; Jumping to the outermost toplevel prompt ;;;and that a lot of text until
Abandon (core dumped)
. I've pushed the new code (i.e. with the algebra as first argument of scalar fields) to the branch of #19147, in case you want to have a look.
Hmm...that is very strange. Perhaps something with pickling and the interface?
However, I starting to lean towards reinstating the UniqueRepresentation
behavior with initialization using a large random number via manifold_constructor
. This will keep memory down with pickling because of all of the extra data that would need to be initialized after every pickling within the session. In an ideal world, I think we would implement our own variable system for the manifolds and hold some kind of global reference to that. I will think about this on my way home right now.
Question, should override the __copy__
for the manifold (at least, I feel there might be some use cases for building two manifolds with a common base atlas)?
comment:83 followup: ↓ 85 Changed 4 years ago by
Okay, let's revert back to having UniqueRepresentation
because it will work as a session/variable manager. I don't think we should spend a lot of time doing that as we can hack around it (even though it is in effect what we are doing/should do). I don't like it because it is a hack, but it seems like the best option for now. Unless you want to directly use Python's multiprocessing with shared memory for the corresponding parents to avoid pickling, which I would still somewhat prefer in a way so we don't have to do this hack. (Multithreading in Python has shared memory, but unfortunately it can't run on multiple processors because of the GIL.) Anyways, I leave the decision up to you.
comment:84 in reply to: ↑ 82 Changed 4 years ago by
Replying to tscrim:
However, I starting to lean towards reinstating the
UniqueRepresentation
behavior with initialization using a large random number viamanifold_constructor
. This will keep memory down with pickling because of all of the extra data that would need to be initialized after every pickling within the session.
OK.
In an ideal world, I think we would implement our own variable system for the manifolds and hold some kind of global reference to that.
Yes this could be something to implement in the future.
Question, should override the
__copy__
for the manifold (at least, I feel there might be some use cases for building two manifolds with a common base atlas)?
Yes, a use case could be trying different things by extending the atlas of a given manifold. The initial manifold, arising for instance from some manifold catalog, could be copied to serve as a reference for the various trials.
comment:85 in reply to: ↑ 83 Changed 4 years ago by
Replying to tscrim:
Okay, let's revert back to having
UniqueRepresentation
because it will work as a session/variable manager. I don't think we should spend a lot of time doing that as we can hack around it (even though it is in effect what we are doing/should do). I don't like it because it is a hack, but it seems like the best option for now. Unless you want to directly use Python's multiprocessing with shared memory for the corresponding parents to avoid pickling, which I would still somewhat prefer in a way so we don't have to do this hack. (Multithreading in Python has shared memory, but unfortunately it can't run on multiple processors because of the GIL.) Anyways, I leave the decision up to you.
Yes, I think it is reasonable to revert to UniqueRepresentation
at this stage, leaving Python's multiprocessing without pickling for a future development, exploring meanwhile other ways of parallelization (IPython parallel framework ?). I've rerun this morning some benchmarks on the commit of #19209 based on UniqueRepresentation
(commit 82f6f495b) merged into Sage 6.10.beta6: the gain in the computation of the Riemann tensor of a concrete metric (4dimensional Kerr metric) is really significant: a factor of 4 when using 8 cores instead of 1.
comment:86 Changed 4 years ago by
 Branch changed from u/tscrim/top_manifolds_refactor to public/manifolds/top_manif_basics
 Commit changed from 0fb39df7fafe7f0a765bf73b3f34a6cb41e65c40 to c5f35afa41b2dac89471ba63d80fd2ae8eebbc2f
 Milestone changed from sage6.10 to sage6.11
I've started to work again on the ticket (not finished yet) and have one question: given the inheritance diagram of comment:72, shall we rename the class Manifold
to TopologicalManifold
? Then, can we revert to Manifold
for manifold_constructor
?
New commits:
d3e5d4d  Revert to UniqueRepresentation for topological manifolds

85d03dc  Change the argument type to structure in Manifold

5251ef0  Remove method _test_pickling from class TopologicalManifoldPoint

2ca24ff  Merge branch 'public/manifolds/top_manif_basics' of git://trac.sagemath.org/sage into Sage 6.10.rc1

cdfa817  Merge branch u/tscrim/top_manifolds_refactor into Sage 6.10.rc1 + public/manifolds/top_manif_basics

c5f35af  Make AbstractSet inherit from UniqueRepresentation; correct doctests; start to change documentation.

comment:87 Changed 4 years ago by
IMO, it is more clear to have in the code manifold_constructor
as that is suppose to be a toplevel function (which we import at Manifold
into the global namespace). Yet I think that is not a strong argument as it introduces a disassociation. Given that we will be using mixin classes, we should revert Manifold
back to TopologicalManifold
. So I leave the other name change up to you.
comment:88 Changed 4 years ago by
 Commit changed from c5f35afa41b2dac89471ba63d80fd2ae8eebbc2f to 3a525002469e0ef676a0dcf48f8f7f8683b85853
Branch pushed to git repo; I updated commit sha1. New commits:
3a52500  Some class renaming; add more examples and doctests (full coverage)

comment:89 followup: ↓ 95 Changed 4 years ago by
In the above commit, I've
 renamed class
Manifold
toTopologicalManifold
 renamed function
manifold_constructor
toManifold
, i.e. to the name by which it is imported to the global namespace (IMHO, this makes the documentation more clear for a beginner: previously,sage: Manifold??
was returning something likedef manifold_constructor
... Also the html doc looks better with a single name)  renamed class
TopologicalSubmanifold
toOpenTopologicalSubmanifold
, to avoid any ambiguity with embedded submanifolds of lower dimension  revert the attribute
_open_covers
to a list of lists (instead of a set of frozensets), mostly to ensure the reproductibility of computations from one machine to another one, i.e. this garantees thatfor ... in
loops are always performed in the same way (maybe this is something to be discussed further...). Moreover, the mathematical definition of an open cover refers to an indexed family, which corresponds more to a list than to a frozenset.  removed the method
is_open()
fromAbstractSet
(where it was always returningTrue
!) and implemented it inTopologicalManifold
. Now it is on the same footing as methodssuperset
,union
andintersection
, which are not implemented in the base classAbstractSet
but in each of the subclassesTopologicalManifold
andManifoldSubset
 removed the (unused) argument
category
fromManifoldSubset.__init__
 added an example to illustrate the class
OpenTopologicalSubmanifold
, as well as enough doctests to get a full coverage
To answer to some of your questions in the code:
 I don't think that
ManifoldPoint
must inherit fromAbstractNamedObject
since the name is not essential in the definition of a point (it can have no name), contrary to that of a subset. Moreover, it would slower the creation of points, which can be a problem (no such problem for parent objects, which are not expected to be massively created)  Yes, I think we need
list_of_subsets
to have something duplicable from one machine to another one (cf. remark above about_open_covers
).
The test suite of ManifoldSubset
and OpenTopologicalSubmanifold
fails because of the lack of a method lift
. What this method shall be? I guess we have to wait for the morphism ticket (#18725) to implement it.
comment:90 Changed 4 years ago by
 Description modified (diff)
comment:91 Changed 4 years ago by
 Commit changed from 3a525002469e0ef676a0dcf48f8f7f8683b85853 to cb534171ae64a7f61a4ea53f41982de0e9eecbd2
Branch pushed to git repo; I updated commit sha1. New commits:
cb53417  Add argument full_name to AbstractNamedObject; remove _repr_() from all parent classes; improve documentation

comment:92 Changed 4 years ago by
In the above commit, the class AbstractNamedObject
has been improved so that there is no need to redefine its method _repr_()
in the classes that inherit from it. Consequently, methods _repr_()
have been removed from all the set and manifold classes. For all these classes, _repr_()
and _latex_()
are now on the same footing: they use the AbstractNamedObject
implementation. This will also be used for the scalar field algebras of #18640.
comment:93 Changed 4 years ago by
 Commit changed from cb534171ae64a7f61a4ea53f41982de0e9eecbd2 to c38ae80cbd8032cf7041259284b6f646265d1e42
Branch pushed to git repo; I updated commit sha1. New commits:
c38ae80  Suppress the (unused) argument category in OpenTopologicalSubmanifold; minor doc improvements

comment:94 Changed 4 years ago by
Status report: I've propagated the refactoring introduced in this ticket up to #18843 (tensor fields on diff. manifolds). In particular, I've introduced the mixin class DifferentiableMixin
in #18783 to deal with differentiable manifolds and open submanifolds, in agreement with the diagram of comment:72. So far, so good...
comment:95 in reply to: ↑ 89 ; followups: ↓ 96 ↓ 103 Changed 4 years ago by
Sorry for the delay in getting to this.
Replying to egourgoulhon:
 revert the attribute
_open_covers
to a list of lists (instead of a set of frozensets), mostly to ensure the reproductibility of computations from one machine to another one, i.e. this garantees thatfor ... in
loops are always performed in the same way (maybe this is something to be discussed further...). Moreover, the mathematical definition of an open cover refers to an indexed family, which corresponds more to a list than to a frozenset.
The set
of frozenset
s will have a faster lookup as manifolds get larger. I think this is still valid mathematically because the indexed family does not have to be ordered as far as I know. So it would be more like a dict
, but I don't think we care about the indexing; if we did, then we should go to a list
. For the doctests, we can just do a sorted
or we have an internal method which returns it in a canonical way (i.e., ordered via its string representation).
 removed the method
is_open()
fromAbstractSet
(where it was always returningTrue
!) and implemented it inTopologicalManifold
.
Perhaps the default value for is_open
in AbstractSet
should return an Unknown
, be an @abstract_method
, or raise a NotImplementedError
? I'm in favor of the first one.
 removed the (unused) argument
category
fromManifoldSubset.__init__
This was previously there as I was wanting to call the __init__
, but since I copied the constructor, this has become unnecessary. Although now I'm thinking we should have an _init_subset
method to remove the duplicated code and have that be called by both ManifoldSubset.__init__
and OpenTopologicalSubmanifold.__init__
.
 added an example to illustrate the class
OpenTopologicalSubmanifold
, as well as enough doctests to get a full coverage
Thank you.
To answer to some of your questions in the code:
 I don't think that
ManifoldPoint
must inherit fromAbstractNamedObject
since the name is not essential in the definition of a point (it can have no name), contrary to that of a subset. Moreover, it would slower the creation of points, which can be a problem (no such problem for parent objects, which are not expected to be massively created)
Fair enough. Although if that extra little function call is going to matter, then I think we should cythonize ManifoldPoint
.
 Yes, I think we need
list_of_subsets
to have something duplicable from one machine to another one (cf. remark above about_open_covers
).
Same comments about _open_covers
as well.
The test suite of
ManifoldSubset
andOpenTopologicalSubmanifold
fails because of the lack of a methodlift
. What this method shall be? I guess we have to wait for the morphism ticket (#18725) to implement it.
lift
can just be a method that creates the corresponding point in the ambient manifold, and similarly for retract
. Once we have #18725, we can convert it to a @lazy_attribute
which is set to the morphism (note that this does not change the API).
I don't reall agree with comment:92. I feel that this only removes a few docstrings, adds some internal complexity, and increases memory usage (although since we aren't really using this for TopologicalPoint
, the memory usage is less of a concern). In principle, it is a mixin class, but it just acts like an ABC. So it is perfectly valid to override its _repr_
.
I'm glad to hear the mixin class idea is working.
comment:96 in reply to: ↑ 95 ; followup: ↓ 97 Changed 4 years ago by
Replying to tscrim:
Sorry for the delay in getting to this.
No problem.
The
set
offrozenset
s will have a faster lookup as manifolds get larger. I think this is still valid mathematically because the indexed family does not have to be ordered as far as I know. So it would be more like adict
, but I don't think we care about the indexing; if we did, then we should go to alist
. For the doctests, we can just do asorted
or we have an internal method which returns it in a canonical way (i.e., ordered via its string representation).
It is not only a matter of doctests, but of reproductability of real computations. For instance the comparison of two vector fields on nonparallelizable manifolds makes use of open covers.
The test suite of
ManifoldSubset
andOpenTopologicalSubmanifold
fails because of the lack of a methodlift
. What this method shall be? I guess we have to wait for the morphism ticket (#18725) to implement it.
lift
can just be a method that creates the corresponding point in the ambient manifold, and similarly forretract
. Once we have #18725, we can convert it to a@lazy_attribute
which is set to the morphism (note that this does not change the API).
OK, thanks.
I don't reall agree with comment:92. I feel that this only removes a few docstrings, adds some internal complexity, and increases memory usage (although since we aren't really using this for
TopologicalPoint
, the memory usage is less of a concern). In principle, it is a mixin class, but it just acts like an ABC. So it is perfectly valid to override its_repr_
.
OK, I will revert to the previous version.
comment:97 in reply to: ↑ 96 ; followup: ↓ 98 Changed 4 years ago by
Replying to egourgoulhon:
Replying to tscrim:
The
set
offrozenset
s will have a faster lookup as manifolds get larger. I think this is still valid mathematically because the indexed family does not have to be ordered as far as I know. So it would be more like adict
, but I don't think we care about the indexing; if we did, then we should go to alist
. For the doctests, we can just do asorted
or we have an internal method which returns it in a canonical way (i.e., ordered via its string representation).It is not only a matter of doctests, but of reproductability of real computations. For instance the comparison of two vector fields on nonparallelizable manifolds makes use of open covers.
But does it depend on the ordering of subsets that give the open cover, or which open cover it takes? Does the API for that have any control over what order it was given (rather than the order in which the open covers were created)? What I'm getting at is that it shouldn't depend upon these things, right? Or are you trying to put a total ordering on these vector fields, and if so, why?
comment:98 in reply to: ↑ 97 Changed 4 years ago by
Replying to tscrim:
Replying to egourgoulhon:
It is not only a matter of doctests, but of reproductability of real computations. For instance the comparison of two vector fields on nonparallelizable manifolds makes use of open covers.
But does it depend on the ordering of subsets that give the open cover, or which open cover it takes?
For this specific case (comparison), it depends on the open cover, not on the ordering of the subsets in it.
Does the API for that have any control over what order it was given (rather than the order in which the open covers were created)?
No there is no control at the API level, because the API is __eq__
(one cannot pass extra parameters)
What I'm getting at is that it shouldn't depend upon these things, right?
Yes, in the ideal mathematical world. In the much less ideal symbolic world, this is not so true... The argument goes as follows: in order to have a pretty fast __eq__
operator for tensor fields, the comparison is not performed on all the subdomains where the tensor fields have known (or computable) restrictions, but only on a single open cover, which is mathematically sufficient. So one has to pick an open cover. By making _open_covers
a list, we ensure that the picked one is always the same. The key point is that sometimes the comparison fails because of simplification issues (no normal form for symbolic expressions), i.e. __eq__
returns False
while the two tensor fields are mathematically equals. This is not satisfactory but I guess this is inescapable when dealing with symbolics. At least, by making _open_covers
a list, we garantee that the output of __eq__
, whatever its mathematical relevance, is consistent from one machine to the next one.
comment:99 followup: ↓ 100 Changed 4 years ago by
Then how about we have a _distinguished_open_cover
, which is set to the first open cover that gets added? IMO, this will be
 lightweight (just a pointer);
 (marginally) faster (only an attribute lookup instead of attribute plus index grab; granted, this speedup will probably never be noticable);
 keep the
O(1)
checking speed of something being an open cover;  and more robust when doing the comparisons (we check to see if the distinguished open cover is an open cover of the other one, so we don't have to worry about things breaking if something ends up being out of order in the creating of the open cover list).
It means one additional attribute we'll have to manage, but I think that is an epsilon cost.
comment:100 in reply to: ↑ 99 ; followup: ↓ 101 Changed 4 years ago by
Replying to tscrim:
Then how about we have a
_distinguished_open_cover
, which is set to the first open cover that gets added?
Well the tensor field __eq__
mentionned above is only one of the possible examples where open covers could be involved. One may imagine that they are also used in tensor field _add_
for instance. This is not the case at the moment, but this is certainly something to be considered when discussing the tensor field arithmetics.
So, at this stage, I would prefer a sequence (i.e. a Python list) of open covers instead of a set with a distinguished element. IMO, this offers a greater flexibility for future operations, like _add_
. With a sequence, you may always say that your distinguished element is the first one, and if, for some reason, the attempted operation fails with it, you may try with the second one, and so on (while if you have only a distinguished element, you don't know what to do in case of failure).
So could we stay with a list at this stage?
comment:101 in reply to: ↑ 100 Changed 4 years ago by
Replying to egourgoulhon:
So could we stay with a list at this stage?
Since you feel strongly about it, then we can leave it be. I got reminded recently about something with premature optimization and the root of all evil... :P
comment:102 Changed 3 years ago by
 Commit changed from c38ae80cbd8032cf7041259284b6f646265d1e42 to 19caedb4d055887ad33ab2ab7b051d166bd58c90
comment:103 in reply to: ↑ 95 Changed 3 years ago by
Replying to tscrim:
I don't reall agree with comment:92. I feel that this only removes a few docstrings, adds some internal complexity, and increases memory usage (although since we aren't really using this for
TopologicalPoint
, the memory usage is less of a concern). In principle, it is a mixin class, but it just acts like an ABC. So it is perfectly valid to override its_repr_
.
In the above commit, I've reverted to the previous version of AbstractNamedObject
and have restored the _repr_
methods.
comment:104 followup: ↓ 106 Changed 3 years ago by
Okay,thanks. So the only thing left to do is implement
def lift(self, p): return self._manifold(p) def retract(self, p): return self(p)
for the subset class (not that we will really use them). Once that is done, I believe the only thing left is for me to take one last look through everything. Thank you for all your work on this.
comment:105 Changed 3 years ago by
 Commit changed from 19caedb4d055887ad33ab2ab7b051d166bd58c90 to 3cd03a48d847e12745ed8c25b23f19db141c179a
Branch pushed to git repo; I updated commit sha1. New commits:
3cd03a4  Add methods lift() and retract() to ManifoldSubset; add method __eq__() in CoordChange

comment:106 in reply to: ↑ 104 Changed 3 years ago by
The above commit implements the methods lift
and retract
as you suggested; it also implements the method __eq__
in class CoordChange
, which fixes the _test_pickling
of that class.
comment:107 Changed 3 years ago by
I've continued to propagate the refactorization initialized in comment:67 up to #19124 (curves in differentiable manifolds) in the ticket hierarchy of #18528. At this stage, the class hierarchy is an extension of the second diagram of comment:72:
Hierarchy1: AbstractSet / \ ManifoldSubset TopologicalManifold  ____/   /  OpenTopSubmanifold     DifferentiableMixin   / \  OpenDiffSubmanifold DifferentiableManifold    IntervalMixin   / \  OpenSubinterval OpenInterval  RealLine
Now, prior to the refactoring, the class hierarchy was
Hierarchy2: ManifoldSubset  TopologicalManifold  DifferentiableManifold  OpenInterval  RealLine
It is clear that Hierarchy2 is much simpler than Hierarchy1: it involves much less classes and has no multiple heritage issues. Moreover, Hierarchy2 reflects fully the mathematics: the real line R is the open interval (oo, +oo), which is a 1dimensional differentiable manifold, which is a 1dimensional topological manifold, which is a subset of a topological manifold (itself). Within Hierarchy2, open subsets are not handled by a specific class, but directly by the manifold classes: an open subset of a topological (resp. differentiable) manifold is created as an instance of TopologicalManifold
(resp. DifferentiableManifold
). Again, this reflects the mathematics since an open subset of a manifold inherits the manifold structure.
The classes of Hierarchy2 have an attribute _manifold
(which could also be called _ambient
), which is the only piece that permits to distinguish between a manifold per se (_manifold
is then set to self
) and an open subset of a larger manifold (_manifold
is then set to the latter). In Hierarchy1, the attribute _manifold
exists only for the subset classes. For motivating Hierarchy1, you said that it avoids tests of the type self is self._manifold
. Now, in Hierarchy2, these tests appear only in two places: (i) the _repr_
method (to return "Open subset of..." instead of "Manifold...") and (ii) the union
and intersection
methods. Hence the question: are there any other reason to introduce Hierarchy1? Given its complexity, as compared with Hierarchy2, I am wondering whether it is worth continuing with it...
Note that with Hierarchy2, one can still distinguish open subsets from the ambient manifold by the category: Manifolds(K).Subobjects()
for open subsets versus Manifolds(K)
for the ambient manifold. There is no need to have a separate class for this.
Another argument in favor of Hierarchy2 regards the construction of a new manifold by the disjoint union of two manifolds (not implemented yet): suppose we have two manifolds A and B of the same dimension over the same topological field K. We can then form the manifold M = A U B and consider A and B as open subsets of M by simplify changing their attribute _manifold
from self
to M. With Hierarchy1, one would need to create brand new objects of the subset type by copying all the information (atlases, vector frames, etc.) contained in A and B.
comment:108 followup: ↓ 109 Changed 3 years ago by
There are 2 really, really good ideas for having a more complicated class hierarchy: encapsulation and localization.
You had many if self._manifold is self:
statements, which breaks the localization principle; it is acting differently because it is essentially a different object. This is a strong code smell and you had enough of these at this level to (IMO) warrant splitting the classes.
Additionally, subsets of a manifold are precisely that, subsets. So we should encapsulate that in separate classes. Mathematically we identity an open subset as a manifold, but by doing so, we loose the information that it is really a subset of another manifold. As such, I feel that having separate classes actually better reflects the mathematics.
In an ideal world, I would like to have a hierarchy similar to Hierachy2, but with an additional subclass corresponding to each type of manifold that is obtained by adding a subset mixin class. Yet, that doesn't seem possible with the current core design.
While the Hierarchy1 seems more complicated, it actually will be easier to maintain and debug as code is local to the class (i.e., if you are a subclass of A
, then you act in the same way and like A
irregardless of your input).
This assessment is based on my experiences and what I learned as good OOP principles. However, I have not looked at SageManifolds beyond this ticket at present, whereas you have worked heavily on it. So perhaps we can get some more data. How many times before the refactoring did you have to do a self._manifold is self
test? Also what is your thoughts on maintenance and extendability?
Something else might be to get a 3rd party's opinion (Nicolas and/or Darij comes to my mind). I could also sit down and plan out how I would do everything from the ground up to see if I can devise a better overall strategy if you think there might be some benefit to that at this stage.
comment:109 in reply to: ↑ 108 Changed 3 years ago by
Replying to tscrim:
You had many
if self._manifold is self:
statements, which breaks the localization principle;
Well not so many: grep returns only 5 occurences in the whole SageManifolds:
 in
TopologicalManifold._repr_
for returning "Manifold..." instead of "Open subset of manifold..."  in
DifferentiableManifold._repr_
for the same reason  in
ManifoldSubset.superset
, because a superset can only beself
in case of the whole manifold  in
ManifoldSubset.intersection
, because the intersection with the whole manifold is trivial  in
ManifoldSubset.union
, because the union with the whole manifold is trivial
Additionally, subsets of a manifold are precisely that, subsets. So we should encapsulate that in separate classes. Mathematically we identity an open subset as a manifold, but by doing so, we loose the information that it is really a subset of another manifold. As such, I feel that having separate classes actually better reflects the mathematics.
Actually, at the moment, we are dealing with the open subsets in exactly the same way as we are doing with the whole manifold, i.e. we set up charts on them, define tensor fields, etc. This can be seen by noticing that in Hierarchy1, the class OpenTopSubmanifold
has very few methods in addition to those inherited from TopologicalManifold
. Similarly, none of the classes OpenDiffSubmanifold
and DifferentiableManifold
introduces any attribute or method by itself, in addition to those inherited from DifferentiableMixin
(except for their __init__
).
Maybe, in a future development, one could have operations that make sense only for open subsets or only for the whole manifold. Then having two separate classes is clearly the way to go. However, at the moment I don't see any such operation, apart from those listed above: the printout (via _repr_
) and the union/intersection with other subsets.
In an ideal world, I would like to have a hierarchy similar to Hierachy2, but with an additional subclass corresponding to each type of manifold that is obtained by adding a subset mixin class. Yet, that doesn't seem possible with the current core design.
While the Hierarchy1 seems more complicated, it actually will be easier to maintain and debug as code is local to the class (i.e., if you are a subclass of
A
, then you act in the same way and likeA
irregardless of your input).This assessment is based on my experiences and what I learned as good OOP principles. However, I have not looked at SageManifolds beyond this ticket at present, whereas you have worked heavily on it. So perhaps we can get some more data. How many times before the refactoring did you have to do a
self._manifold is self
test?
Five times, as mentioned above.
Also what is your thoughts on maintenance and extendability?
It seems to me that, in terms of maintenance, Hierarchy2 is better, being simpler; it is also certainly better for somebody new entering into the code. On the contrary, if one regards extendability, Hierarchy1 offers the possibility to have different operations for manifolds and subsets, as mentioned above. This is a good point in favor of it. Even if at the moment I don't have in mind any such operation (apart from those listed above), I would slightly tend to maintain Hierarchy1 because of this.
Something else might be to get a 3rd party's opinion (Nicolas and/or Darij comes to my mind).
Yes extra points of view would be helpful! In addition to Nicolas and Darij, I don't know if other people associated to this ticket (Vincent, John, Nils, Jeoren,...) are reading this and have an opinion...
I could also sit down and plan out how I would do everything from the ground up to see if I can devise a better overall strategy if you think there might be some benefit to that at this stage.
Thanks, but maybe this is not necessary at this stage, especially if we conclude that we maintain Hierarchy1. Since all this has little impact on the user interface, maybe we could rediscuss it after having a whole view of all the manifold tickets.
comment:110 Changed 3 years ago by
Giving a second thought to this, another hierarchy that would preserve the distinction between whole manifolds and open subsets is
Hierarchy3: AbstractAmbient ManifoldSubset        OpenTopSubmanifold   \ /    TopologicalManifold       OpenDiffSubmanifold  \ /   DifferentiableManifold     OpenSubinterval \____ ____/ OpenInterval  RealLine
The class AbstractAmbient
would only implement the methods union
and intersection
, which are trivial in this case. Each of the classes TopologicalManifold
, DifferentiableManifold
and OpenInterval
would implement only the method _repr_
.
Hierarchy3 is simpler than Hierarchy1 and does not require any mixin class.
It is also easy to add a new structure, like complex manifolds.
Hierarchy3 is also mathematically neat, since a topological (resp. differentiable) manifold is obviously a open subset of a topological (resp. differentiable) manifold. In this respect it reverses the logic of Hierarchy1, where the class OpenTopSubmanifold
inherits from TopologicalManifold
, not the opposite. Maybe the latter logic is quite well spread for algebraic structures in Sage, I mean classes for substructures inheriting from classes for the ambient structure. But for topology, the reverse logic, as proposed in Hierarchy3, could be more adapted: a topological space is often treated as an open subset of itself. For instance, this occurs in its very definition: a topological space is a set X endowed with a collection of subsets of X, called the open subsets, such that the empty set and X are open, etc.
What do you think?
comment:111 Changed 3 years ago by
Here is another proposal, which is a kind of mix between Hierarchy1 and Hierarchy3:
Hierarchy4: AbstractAmbient AbstractSet AbstractSubset     \ /        ManifoldSubset              TopologicalManifoldOpenSet      \ /  \ /     TopologicalManifold  OpenTopSubmanifold          DifferentiableManifoldOpenSet    \ /  \ /   DifferentiableManifold  OpenDiffSubmanifold      IntervalOpenSet / \____ ____/ \ __________/ OpenInterval OpenSubInterval  RealLine
Contrary to Hierarchy1, it has no diamond problem and involves no mixin class.
As in Hierarchy3, the class AbstractAmbient
implements only the methods union
and intersection
, but contrary to Hierarchy3, the classes for ambient objects (TopologicalManifold
, DifferentiableManifold
, etc.) do no longer have the attribute _manifold
. This attribute is present only in the subobject classes OpenTopSubmanifold
, OpenDiffSubmanifold
and OpenSubinterval
, as in Hierarchy1. In Hierarchy4, the attribute _manifold
is inherited from AbstractSubset
. The class ManifoldSubset
implements generic (not open) subsets of a manifold.
The classes TopologicalManifold
and OpenTopSubmanifold
do not implement
any new method by themselves: most methods are implemented in their parent
abstract class TopologicalManifoldOpenSet
, like chart()
, atlas()
,
scalar_field_algebra()
, etc.
Similarly, methods for differentiable manifolds, like vector_frame()
or
tensor_field()
, are implemented in the abstract class DifferentiableManifoldOpenSet
.
comment:112 followup: ↓ 113 Changed 3 years ago by
I think if we are really going to go through with what might be some hard work and separate out the subset features, we should just do this:
AbstractSet AbstractSubsetMixin      /   TopologicalManifold /    \_____ / /   \ / /   TopologicalSubmanifold /  DifferentiableManifold / /  \__ / /  \ / /  DifferentiableSubmanifold / OpenInterval /  \__________ ____________/  \ /  OpenSubinterval RealLine
It is what I was hoping to obtain, but had trouble separating out the subset portion because there still is a small diamond problem with Parent
. Which is why AbstractSubset
would have to be a mixin, both in this hierarchy and Hierarchy3,4 to be effective.
It does seem like perhaps Hierarchy2 would be the best since there is so few tests for if self._manifold is self
. The biggest trouble I have with this is the separations of concerns: that it makes it somewhat harder to separate these classes later if they start needing different attributes. We also have wellestablished code in Sage for which there is a different class for subobjects.
Ah hell, perhaps we should just revert back to Hierarchy2 to keep the simplicity. I'm starting to wonder if there is perhaps a more fundamental issue in that we are trying to be too close to the mathematical definitions rather than be programmers, which could potentially be a complete rewrite of most things. However, we have working code, which is always better than no code. Anyways, I'm going to shutup now and just ask do you want me to handle the revert or are you willing to do it?
comment:113 in reply to: ↑ 112 Changed 3 years ago by
Replying to tscrim:
It is what I was hoping to obtain, but had trouble separating out the subset portion because there still is a small diamond problem with
Parent
. Which is whyAbstractSubset
would have to be a mixin, both in this hierarchy and Hierarchy3,4 to be effective.
Yes. Similarly AbstractAmbient
in Hierarchy4 has to be a mixin, so that only AbstractSet
inheritates from Parent
.
It does seem like perhaps Hierarchy2 would be the best since there is so few tests for
if self._manifold is self
. The biggest trouble I have with this is the separations of concerns: that it makes it somewhat harder to separate these classes later if they start needing different attributes. We also have wellestablished code in Sage for which there is a different class for subobjects.
As said before, it is difficult to see at the monent what kind of different attributes would exist for manifolds and open submanifolds. Basically, our open submanifolds behave just like manifolds, especially when defining fields on them. This reflects the fact that both are open sets, which is the true basic structure in topology. Things are probably different in other parts of Sage, which deal with algebraic structures, instead of topological ones.
Ah hell, perhaps we should just revert back to Hierarchy2 to keep the simplicity. I'm starting to wonder if there is perhaps a more fundamental issue in that we are trying to be too close to the mathematical definitions rather than be programmers, which could potentially be a complete rewrite of most things. However, we have working code, which is always better than no code.
OK, let us revert to Hierarchy2. It is simple, effective, offers a better gathering of documentation, and, via self._manifold = self
, it embodies the fact that a manifold is an open subset of itself, which is not a totally crazy mathematical idea.
Anyway, the whole discussion has been very interesting (at least to me) and we can keep the above diagrams for future thoughts.
Anyways, I'm going to shutup now and just ask do you want me to handle the revert or are you willing to do it?
I will do it. Of course, it will not be a complete revert to the state prior to your refactorization: we shall keep your singleton classes for the structure (topological, differentiable, etc.) and your removal of facade parents, which are nice features.
comment:114 Changed 3 years ago by
 Commit changed from 3cd03a48d847e12745ed8c25b23f19db141c179a to 984c3c26bf827f44eee26fc6afd321e11dca8f2e
Branch pushed to git repo; I updated commit sha1. New commits:
984c3c2  Revert to simple hierarchy for manifold classes

comment:115 Changed 3 years ago by
 Status changed from needs_info to needs_review
Following comment:113, the above commit reverts to Hierarchy2 (only the first two items of it, ManifoldSubset
and TopologicalManifold
are implemented in this ticket).
comment:116 Changed 3 years ago by
 Description modified (diff)
comment:117 followup: ↓ 118 Changed 3 years ago by
I also think this discussion was good. I'm just slightly frustrated at myself for not being able to come up with a clear better alternative to get rid of the code smell.
Anyways, now it is back to going over the documentation and technicalities of the code. I hope to finish this review in a couple of days. I'm also thinking that the followup tickets will probably be a lot easier/faster to review too.
comment:118 in reply to: ↑ 117 Changed 3 years ago by
Replying to tscrim:
I also think this discussion was good. I'm just slightly frustrated at myself for not being able to come up with a clear better alternative to get rid of the code smell.
I think there is still the opportunity to implement another hierarchy latter, if we feel that specificities for subsets are required. Also, if we do this latter, when more tickets of #18528 are merged, this would alleviate the propagation of the changes to other tickets and one would notice at once possible side effects.
Anyways, now it is back to going over the documentation and technicalities of the code. I hope to finish this review in a couple of days.
Thanks.
I'm also thinking that the followup tickets will probably be a lot easier/faster to review too.
I've propagated the latest changes in this ticket to all tickets of #18528. Everything is OK. In particular, the new singleton classes for manifold structures have allowed to simplify the code, avoiding to redefine the methods chart
and scalar_field_algebra
in the class DifferentiableManifold
.
comment:119 Changed 3 years ago by
 Commit changed from 984c3c26bf827f44eee26fc6afd321e11dca8f2e to 4ed37cece6c44c8c55e79b4dd9990eafe29c20d9
comment:120 followup: ↓ 123 Changed 3 years ago by
 Milestone changed from sage7.0 to sage7.1
 Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Eric Gourgoulhon
Okay, I've made my last pass through everything. I have two comments left:
 In
Chart.set_inverse
, there is a keywordcheck
that I changed toverbose
because there was no check being done. Irregardless, I don't quite like that the default is to print things to the terminal. Is there a specific reason for this? I feel like the default should beFalse
.  In
ManifoldPoint
, the methodsset_coord
should be the fullset_coordinates
(and similar methods). The short version can be an alias. (Same discussion as fordisp
vsdisplay
.).
Once we take care of these, then this will be a positive review.
comment:121 Changed 3 years ago by
 Commit changed from 4ed37cece6c44c8c55e79b4dd9990eafe29c20d9 to cf5e98b2041d1fdee0756e99a85ada0b865375da
Branch pushed to git repo; I updated commit sha1. New commits:
cf5e98b  Use standard copyright template.

comment:122 Changed 3 years ago by
 Commit changed from cf5e98b2041d1fdee0756e99a85ada0b865375da to 00d265cf2855121dba914868264da6ea3a9c42af
comment:123 in reply to: ↑ 120 Changed 3 years ago by
Replying to tscrim:
Okay, I've made my last pass through everything.
Thanks a lot for all your changes.
I have two comments left:
 In
Chart.set_inverse
, there is a keywordcheck
that I changed toverbose
because there was no check being done.
Yes, verbose
seems more appropriate.
Irregardless, I don't quite like that the default is to print things to the terminal. Is there a specific reason for this?
No.
I feel like the default should be
False
.
I've set it to False
in the above commit.
 In
ManifoldPoint
, the methodsset_coord
should be the fullset_coordinates
(and similar methods). The short version can be an alias. (Same discussion as fordisp
vsdisplay
.).
I've performed the change in the above commit, adding aliases for short versions of coordinates
, add_coordinates
and set_coordinates
. I've also corrected one or two typos.
comment:124 Changed 3 years ago by
 Status changed from needs_review to positive_review
LGTM. Here is the start of the main body of SageManifolds into Sage. Thank you for all your hard work.
comment:125 Changed 3 years ago by
Thanks a lot for the detailed review work, the numerous improvements and the enlightening discussions!
comment:126 Changed 3 years ago by
 Branch changed from public/manifolds/top_manif_basics to 00d265cf2855121dba914868264da6ea3a9c42af
 Resolution set to fixed
 Status changed from positive_review to closed
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.