Opened 5 years ago

Last modified 4 years ago

#18529 closed enhancement

Topological manifolds: basics — at Version 44

Reported by: egourgoulhon Owned by: egourgoulhon
Priority: major Milestone: sage-7.1
Component: geometry Keywords: topological manifolds
Cc: mmancini Merged in:
Authors: Eric Gourgoulhon Reviewers:
Report Upstream: N/A Work issues:
Branch: public/manifolds/top_manif_basics Commit: 902908b41a95d3455bfcc497997ad2054c530a96
Dependencies: #18175 Stopgaps:

Description (last modified by egourgoulhon)

This is the implementation of topological manifolds over a topological field K resulting from the SageManifolds project. See the meta-ticket #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 Kn, with the same non-negative integer n for all points.

This tickets implements the following Python classes:

  • TopologicalManifold: topological manifold over a topological field K
  • TopologicalManifoldPoint: point in a topological manifold
  • TopologicalManifoldSubset: generic subset of a topological manifold
  • Chart: chart of a topological manifold
    • RealChart: chart of a topological manifold over the real field
  • CoordChange: transition map between two charts of a topological manifold

TopologicalManifold is intended to serve as a base class for specific manifolds, like smooth manifolds (K=R) and complex manifolds (K=C).

Documentation: The reference manual is produced by sage -docbuild reference/manifolds html It can also be accessed online at http://sagemanifolds.obspm.fr/doc/18529/reference/manifolds/ More documentation (e.g. example worksheets) can be found here.

Change History (44)

comment:1 Changed 5 years ago by egourgoulhon

  • Description modified (diff)

comment:2 follow-up: Changed 5 years ago by jhpalmieri

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 non-negative 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.

comment:3 Changed 5 years ago by egourgoulhon

  • Description modified (diff)

comment:4 in reply to: ↑ 2 Changed 5 years ago by egourgoulhon

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 non-negative 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.

Last edited 5 years ago by egourgoulhon (previous) (diff)

comment:5 Changed 5 years ago by git

  • Commit changed from 89c063c7119f19497e3d21b2a5a9dcb0752122b0 to 5a5722b4a0ef33d8624fdd127bbb1964232ced96

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

5a5722bAdd doctests in classes Chart and RealChart

comment:6 Changed 5 years ago by git

  • Commit changed from 5a5722b4a0ef33d8624fdd127bbb1964232ced96 to 4f490af5fedeb0a28dd8ddab70efdab1cc64bf93

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

4f490afImprove the documentation of coordinate charts

comment:7 Changed 5 years ago by git

  • Commit changed from 4f490af5fedeb0a28dd8ddab70efdab1cc64bf93 to d8df59f286da79e1e103b56064fdffb702e034ce

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

d8df59fImprove the documentation of TopManifold

comment:8 Changed 5 years ago by git

  • Commit changed from d8df59f286da79e1e103b56064fdffb702e034ce to fb96562c1e7d4ffc76ca87efcc15d133c6c15190

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

fb96562Open subsets of topological manifolds are now fully considered as topological manifolds.

comment:9 Changed 5 years ago by egourgoulhon

  • Description modified (diff)

comment:10 Changed 5 years ago by git

  • Commit changed from fb96562c1e7d4ffc76ca87efcc15d133c6c15190 to 38c3c12cb1d9b4d52a35ad6f7a1a8d0ee666d532

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

38c3c12Improve documentation of classes TopManifold, TopManifoldSubset and TopManifoldPoint

comment:11 Changed 5 years ago by git

  • Commit changed from 38c3c12cb1d9b4d52a35ad6f7a1a8d0ee666d532 to 7809ebf468f80143e33830e0df75046bf191ce24

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

7809ebfMinor modifications in classes TopManifold and Chart.

comment:12 Changed 5 years ago by git

  • Commit changed from 7809ebf468f80143e33830e0df75046bf191ce24 to 99cc8c1c94197b1648966eaa7b2a6d04ddab1efc

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

99cc8c1Introduce Chart._init_coordinates() to simplify constructors of Chart and RealChart

comment:13 Changed 5 years ago by git

  • Commit changed from 99cc8c1c94197b1648966eaa7b2a6d04ddab1efc to 26fc318bbacfc9a27049a9397d43402975525820

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

26fc318Modifications in CoordChange to allow for subclasses.

comment:14 Changed 5 years ago by git

  • Commit changed from 26fc318bbacfc9a27049a9397d43402975525820 to a74c2e0519c166657b858be5b4b3dc4fd0145d09

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

a74c2e0Add example of p-adic manifold

comment:15 Changed 5 years ago by git

  • Commit changed from a74c2e0519c166657b858be5b4b3dc4fd0145d09 to 542b82a9ed134759fe498cd34b682b2d565061c1

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

542b82aSuppress verbose in TestSuite().run; minor improvements in documentation

comment:16 Changed 5 years ago by git

  • Commit changed from 542b82a9ed134759fe498cd34b682b2d565061c1 to 26c489001c4ee64a95ee70da72210e361180eeb0

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

26c4890Minor improvements in the doc of topological manifolds (basics part)

comment:17 Changed 5 years ago by git

  • Commit changed from 26c489001c4ee64a95ee70da72210e361180eeb0 to be3ff7424963450c1c2dfa7ca9fbe85eaeab1162

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

be3ff74Introduce open covers on top manifolds

comment:18 Changed 5 years ago by git

  • Commit changed from be3ff7424963450c1c2dfa7ca9fbe85eaeab1162 to 4de19a74c83ac6d4d0c4da74e1d1f2afce5c3045

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

4de19a7Merge branch 'public/manifolds/top_manif_basics' of git://trac.sagemath.org/sage into sage 6.9

comment:19 Changed 5 years ago by egourgoulhon

  • Description modified (diff)
  • Milestone changed from sage-6.8 to sage-6.10
  • Status changed from new to needs_review

comment:20 follow-up: Changed 5 years ago by vdelecroix

Why did you abreviate TopologicalManifold? Everywhere in Sage classes have plain names like PolynomialRing and not PolRing.

comment:21 in reply to: ↑ 20 Changed 5 years ago by egourgoulhon

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 not PolRing.

This is indeed a strong point: naming conventions should be homogeneous all across Sage. So I am considering to change everywhere TopManifold to TopologicalManifold and DiffManifold to DifferentiableManifold.

comment:22 Changed 5 years ago by git

  • Commit changed from 4de19a74c83ac6d4d0c4da74e1d1f2afce5c3045 to 6dec6d592a09e56921b9a761827309dd31ae2533

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

3647305Some cleanup and adding more category information to particular sets.
f810aa6Reworking the category of manifolds.
375ff46Fixing doctest failures and letting a few other rings know they are metric spaces.
8b851a0Merge branch 'develop' into public/categories/topological_metric_spaces-18175
f8f5b93Fixing last remaining doctests.
041a5d1Adding p-adics to metric spaces and some cleanup.
bfa0cdfOne last doc tweak.
d13c368Fixing doc of metric spaces.
2605c0bMerge #18529 (Topological manifolds: basics) into #18175 (Implement categories for topological...)
6dec6d5Implement topological manifolds (basics, #18529) on the new categories for manifolds (#18175)

comment:23 follow-up: Changed 5 years ago by tscrim

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 ; follow-up: Changed 5 years ago by egourgoulhon

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 ; follow-up: Changed 5 years ago by tscrim

Replying to egourgoulhon:

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?

I don't know how easy it would be to make this a check on the atlas used to construct the manifold. I would make it select it automatically if possible, and then default to the most general setting of 'topological'.

Besides, I remember someone was complaining that Manifold was already used in SnapPy, cf. this page, but is there any danger of name clash? i.e. is SnapPy used (or could be used in the future) from Sage?

I would then add an additional argument of implementation, which would default to the native Sage implementation, if there does become a name clash and have the constructor delegate out to the SnapPy version when it exists in Sage (IDK if it is Sage now or not).

comment:26 in reply to: ↑ 25 Changed 5 years ago by egourgoulhon

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 follow-up: Changed 5 years ago by tscrim

Some things from looking over the current structure:

  • I would separate out parts of the subset class that applies to Top(ological)Manifold and TopManifoldSubset into an ABC (abstract base class) so you don't have to do things like self 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 like TopManifold._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 by is). This would alleviate the need for UniqueRepresentation.
  • In the tests for _element_constructor_, you shouldn't call it explicitly but via X(...), 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 check isinstance(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' to RR (and 'complex' to CC). 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 underlying SR implementation.

comment:28 in reply to: ↑ 27 ; follow-ups: Changed 5 years ago by 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 and TopManifoldSubset into an ABC (abstract base class) so you don't have to do things like self 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 like TopManifold._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 by is). This would alleviate the need for UniqueRepresentation.

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 non-doctest case involving the garbage collector that you mentioned)

  • In the tests for _element_constructor_, you shouldn't call it explicitly but via X(...), 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 check isinstance(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' to RR (and 'complex' to CC). 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 underlying SR implementation.

Yes, I agree too.

comment:29 in reply to: ↑ 28 Changed 5 years ago by egourgoulhon

Replying to egourgoulhon:

Replying to tscrim:

  • I would separate out parts of the subset class that applies to Top(ological)Manifold and TopManifoldSubset into an ABC (abstract base class) so you don't have to do things like self 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.

I gave a second thought to this: are you thinking about something like

                               The_ABC
                              /      \
  TopologicalManifoldStrictSubset  TopologicalManifold
                              \      /
                 TopologicalManifoldStrictOpenSubset

with the methods superset(), intersection() and union() being implemented in each of the classes TopologicalManifoldStrictSubset and TopologicalManifold ?

comment:30 Changed 5 years ago by tscrim

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 ; follow-up: Changed 5 years ago by tscrim

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 and TopManifoldSubset into an ABC (abstract base class) so you don't have to do things like self 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.

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

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 like TopManifold._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.

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 by is). This would alleviate the need for UniqueRepresentation.

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 non-doctest case involving the garbage collector that you mentioned)

This comes down to the manifold not being well-defined 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 ; follow-up: Changed 5 years ago by 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.

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 by is). This would alleviate the need for UniqueRepresentation.

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 non-doctest case involving the garbage collector that you mentioned)

This comes down to the manifold not being well-defined 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) 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).

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 ; follow-up: Changed 5 years ago by tscrim

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

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 ; follow-up: Changed 5 years ago by 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.

comment:35 in reply to: ↑ 34 Changed 5 years ago by tscrim

Replying to egourgoulhon:

Replying to tscrim:

As I outlined above, it should be easy enough to remove the UniqueRepresentation behavior and still retain the desirable behaviors such as hashability.

I am on it and should push a new commit soon.

Thanks. Also thank you for all your work on this.

comment:36 Changed 5 years ago by git

  • Commit changed from 6dec6d592a09e56921b9a761827309dd31ae2533 to f342e03e7008831c4789b94b03674c1a0cbbf3a6

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

f582241Introduce function Manifold() as the global entry point to construct any type of manifold.
f342e03Remove UniqueRepresentation from topological manifolds, subsets and charts.

comment:37 Changed 5 years ago by egourgoulhon

The above commit addresses some of the comments by Travis and Vincent:

  • The class TopManifold has been renamed TopologicalManifold and the class TopManifoldSubset has been renamed TopologicalManifoldSubset.
  • 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 and RealChart do not longer inherit from UniqueRepresentation. 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 class Chart 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 attribute self._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 and CC)

comment:38 follow-up: Changed 5 years ago by 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 (and we can somewhat short circuit this by first checking self is other [which is also a general python recommendation]).

comment:39 in reply to: ↑ 38 Changed 5 years ago by egourgoulhon

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 follow-up: Changed 5 years ago by 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. Moreover, a sophisticated equality check (either with __eq__ or is_isomorphic_to) seems difficult to acheive: checking the equality of the user-defined 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 S2 with an atlas of two stereographic charts and then another S2 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?

Last edited 5 years ago by egourgoulhon (previous) (diff)

comment:41 in reply to: ↑ 40 ; follow-up: Changed 5 years ago by 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__ 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__ or is_isomorphic_to) seems difficult to acheive: checking the equality of the user-defined 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 S2 with an atlas of two stereographic charts and then another S2 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 Rn -> 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 function Manifold, so that

  M1 = 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 use sage.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 a UniqueFactory).
  • 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 sage-devel, for other options.

Personally I would go for first one since pickling within a session is, I believe, usually not needed/used.

comment:42 Changed 5 years ago by git

  • Commit changed from f342e03e7008831c4789b94b03674c1a0cbbf3a6 to 902908b41a95d3455bfcc497997ad2054c530a96

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

902908bRevert to UniqueRepresentation for topological manifolds and charts, with the possibility to reuse manifold names.

comment:43 in reply to: ↑ 41 Changed 5 years ago by egourgoulhon

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__ being

if 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 function Manifold, so that

  M1 = 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.

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 use sage.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.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 a UniqueFactory).

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 sage-devel, for other options.

Good idea, I will.

comment:44 Changed 5 years ago by egourgoulhon

  • Description modified (diff)
Note: See TracTickets for help on using tickets.