#18529 closed enhancement (fixed)

Topological manifolds: basics

Reported by: egourgoulhon Owned by: egourgoulhon
Priority: major Milestone: sage-7.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 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:

  • ManifoldSubset: generic subset of a topological manifold (the open subsets being implemented by the subsclass TopologicalManifold)
    • TopologicalManifold: topological manifold over a topological field K
  • ManifoldPoint: point in 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

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 follow-up 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 22 months ago by egourgoulhon

  • Description modified (diff)

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

  • Description modified (diff)

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

comment:5 Changed 22 months 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 22 months 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 22 months 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 22 months 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 22 months ago by egourgoulhon

  • Description modified (diff)

comment:10 Changed 22 months 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 22 months 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 22 months 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 21 months 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 21 months 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 19 months 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 18 months 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 18 months 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 18 months 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 18 months 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 18 months 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 18 months 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 17 months 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 17 months 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 17 months 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 17 months 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 17 months 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-ups: Changed 17 months 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 17 months 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 17 months 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 17 months 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 17 months 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 17 months 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 17 months 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 17 months 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 17 months 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 17 months 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 follow-up: Changed 17 months 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 17 months 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 17 months 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 17 months 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 17 months ago by egourgoulhon (previous) (diff)

comment:41 in reply to: ↑ 40 ; follow-up: Changed 17 months 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 17 months 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 17 months 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 17 months ago by egourgoulhon

  • Description modified (diff)

comment:45 in reply to: ↑ 37 ; follow-up: Changed 17 months ago by 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 test loads(dumps(M)) == M would failed, since obviously id(loads(dumps(M)) differs from id(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 17 months ago by egourgoulhon

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 test loads(dumps(M)) == M would failed, since obviously id(loads(dumps(M)) differs from id(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 follow-up: Changed 17 months ago by jdemeyer

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.

Last edited 17 months ago by jdemeyer (previous) (diff)

comment:48 follow-up: Changed 17 months ago by jdemeyer

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 17 months ago by egourgoulhon

Replying to jdemeyer:

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.

OK I will change this. Thanks.

comment:50 in reply to: ↑ 48 Changed 17 months ago by egourgoulhon

Replying to jdemeyer:

In think this also holds for type(foo) instead if foo.__class__ but I'm not 100% sure if those are really equivalent.

From this link and that one, it seems that for Python new-style 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 17 months ago by git

  • Commit changed from 902908b41a95d3455bfcc497997ad2054c530a96 to 252e616cc053a3b76ee563282222507cd78c9fb8

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

252e616Remove UniqueRepresentation, leaving only WithEqualityById, for topological manifolds and charts.

comment:52 Changed 17 months ago by egourgoulhon

  • Description modified (diff)

The above commit takes into account this discussion on sage-devel, 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 equality-by-id 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 17 months ago by egourgoulhon

Replying to tscrim:

  • 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.

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 follow-up: Changed 17 months ago by 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 of Manifolds(RR) and Manifolds(CC).

Last edited 17 months ago by tscrim (previous) (diff)

comment:55 in reply to: ↑ 54 ; follow-up: Changed 17 months ago by 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 of Manifolds(RR) and Manifolds(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 subclass RealChart of the generic class Chart.
  • _field, which contains the field for the manifold category; it could default to RR for _field_type == 'real'.

Do you agree?

comment:56 in reply to: ↑ 55 Changed 17 months ago by tscrim

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 of Manifolds(RR) and Manifolds(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 subclass RealChart of the generic class Chart.
  • _field, which contains the field for the manifold category; it could default to RR 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 16 months ago by git

  • Commit changed from 252e616cc053a3b76ee563282222507cd78c9fb8 to 65186990b1106f89652107356b60faa048113915

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

6518699Introduce the attribute _field_type in class TopologicalManifold to check for real and complex manifolds.

comment:58 Changed 16 months ago by git

  • Commit changed from 65186990b1106f89652107356b60faa048113915 to 0b08b114e03c063bd2e500ac900fd843fb12673b

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

d8397c1Merge branch 'public/manifolds/top_manif_basics' of trac.sagemath.org:sage into public/manifolds/top_manif_basics
0b08b11Some small tweaks as part of the review.

comment:59 follow-up: Changed 16 months ago by tscrim

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 16 months ago by tscrim

  • Reviewers set to Travis Scrimshaw

comment:61 in reply to: ↑ 59 ; follow-ups: Changed 16 months ago by egourgoulhon

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

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

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, [x-2,y-2])
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 ; follow-up: Changed 16 months ago by egourgoulhon

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 cherry-pick 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 16 months ago by tscrim

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

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 off-hand, then I wouldn't worry about it.

comment:66 in reply to: ↑ 65 Changed 16 months ago by egourgoulhon

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 follow-up: Changed 16 months ago by tscrim

  • 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:

0fb39dfRefactoring the code to separate out the structural part of the manifold.

comment:68 in reply to: ↑ 67 ; follow-up: Changed 16 months ago by egourgoulhon

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

Replying to egourgoulhon:

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?

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

Good point. Then rather these being instance objects, they should probably be mix-in 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 ; follow-up: Changed 16 months ago by egourgoulhon

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 C2-structure differs from a C4-structure.

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?

Good point. Then rather these being instance objects, they should probably be mix-in 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 mix-in classes? and why this would be superior to the simple heritage TopologicalManifold <-- DifferentiableManifold ?

comment:71 Changed 16 months ago by egourgoulhon

  • Authors changed from Eric Gourgoulhon to Eric Gourgoulhon, Travis Scrimshaw

comment:72 in reply to: ↑ 70 ; follow-up: Changed 16 months ago by tscrim

Replying to egourgoulhon:

Could you please describe further how you would use mix-in 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 16 months ago by egourgoulhon

Thanks for these explanations.

comment:74 in reply to: ↑ 61 Changed 16 months ago by egourgoulhon

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 3-dimensional differentiable manifold M) and y (=Scalar field on the 3-dimensional 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 3-dimensional differentiable manifold M'
whereas y has parent 'Algebra of differentiable scalar fields on the 3-dimensional 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 follow-up: Changed 16 months ago by 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.

comment:76 in reply to: ↑ 75 ; follow-up: Changed 16 months ago by egourgoulhon

  • 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 front-end 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 sage-devel 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 ; follow-up: Changed 16 months ago by tscrim

Replying to egourgoulhon:

Another solution would be to revert to UniqueRepresentation for manifolds (once again!). Since now the user creates manifolds via the front-end 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.

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

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 16 months ago by egourgoulhon

Marco and I have discussed the thing this morning. It's clear that the multiprocessing Python module relies on pickling; some references are

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 sage-devel 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 follow-up: Changed 16 months ago by tscrim

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

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 Ck(M) to Ck(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 ; follow-up: Changed 16 months ago by tscrim

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 Ck(M) to Ck(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.

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

comment:84 in reply to: ↑ 82 Changed 16 months ago by egourgoulhon

Replying to tscrim:

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.

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 16 months ago by egourgoulhon

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 non-trivial metric (4-dimensional Kerr metric) is really significant: a factor of 4 when using 8 cores instead of 1 (30 s instead of 115 s on Xeon E5-2623 CPUs).

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

comment:86 Changed 16 months ago by egourgoulhon

  • Branch changed from u/tscrim/top_manifolds_refactor to public/manifolds/top_manif_basics
  • Commit changed from 0fb39df7fafe7f0a765bf73b3f34a6cb41e65c40 to c5f35afa41b2dac89471ba63d80fd2ae8eebbc2f
  • Milestone changed from sage-6.10 to sage-6.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:

d3e5d4dRevert to UniqueRepresentation for topological manifolds
85d03dcChange the argument type to structure in Manifold
5251ef0Remove method _test_pickling from class TopologicalManifoldPoint
2ca24ffMerge branch 'public/manifolds/top_manif_basics' of git://trac.sagemath.org/sage into Sage 6.10.rc1
cdfa817Merge branch u/tscrim/top_manifolds_refactor into Sage 6.10.rc1 + public/manifolds/top_manif_basics
c5f35afMake AbstractSet inherit from UniqueRepresentation; correct doctests; start to change documentation.

comment:87 Changed 16 months ago by tscrim

IMO, it is more clear to have in the code manifold_constructor as that is suppose to be a top-level 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 16 months ago by git

  • Commit changed from c5f35afa41b2dac89471ba63d80fd2ae8eebbc2f to 3a525002469e0ef676a0dcf48f8f7f8683b85853

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

3a52500Some class renaming; add more examples and doctests (full coverage)

comment:89 follow-up: Changed 16 months ago by egourgoulhon

In the above commit, I've

  • renamed class Manifold to TopologicalManifold
  • renamed function manifold_constructor to Manifold, 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 like def manifold_constructor... Also the html doc looks better with a single name)
  • renamed class TopologicalSubmanifold to OpenTopologicalSubmanifold, 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 that for ... 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() from AbstractSet (where it was always returning True!) and implemented it in TopologicalManifold. Now it is on the same footing as methods superset, union and intersection, which are not implemented in the base class AbstractSet but in each of the subclasses TopologicalManifold and ManifoldSubset
  • removed the (unused) argument category from ManifoldSubset.__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 from AbstractNamedObject 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 16 months ago by egourgoulhon

  • Description modified (diff)

comment:91 Changed 16 months ago by git

  • Commit changed from 3a525002469e0ef676a0dcf48f8f7f8683b85853 to cb534171ae64a7f61a4ea53f41982de0e9eecbd2

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

cb53417Add argument full_name to AbstractNamedObject; remove _repr_() from all parent classes; improve documentation

comment:92 Changed 16 months ago by egourgoulhon

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 15 months ago by git

  • Commit changed from cb534171ae64a7f61a4ea53f41982de0e9eecbd2 to c38ae80cbd8032cf7041259284b6f646265d1e42

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

c38ae80Suppress the (unused) argument category in OpenTopologicalSubmanifold; minor doc improvements

comment:94 Changed 15 months ago by egourgoulhon

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 ; follow-ups: Changed 15 months ago by tscrim

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 that for ... 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 frozensets 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() from AbstractSet (where it was always returning True!) and implemented it in TopologicalManifold.

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 from ManifoldSubset.__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 from AbstractNamedObject 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 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.

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

Replying to tscrim:

Sorry for the delay in getting to this.

No problem.

The set of frozensets 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).

It is not only a matter of doctests, but of reproductability of real computations. For instance the comparison of two vector fields on non-parallelizable manifolds makes use of 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.

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

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

Replying to egourgoulhon:

Replying to tscrim:

The set of frozensets 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).

It is not only a matter of doctests, but of reproductability of real computations. For instance the comparison of two vector fields on non-parallelizable 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 15 months ago by egourgoulhon

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 non-parallelizable 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 follow-up: Changed 15 months ago by tscrim

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

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 15 months ago by tscrim

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 15 months ago by git

  • Commit changed from c38ae80cbd8032cf7041259284b6f646265d1e42 to 19caedb4d055887ad33ab2ab7b051d166bd58c90

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

069baf4Merge branch 'public/manifolds/top_manif_basics' into Sage 7.0.beta2.
19caedbRevert to AbstractNamedObject without argument full_name; restore _repr_() in manifold classes

comment:103 in reply to: ↑ 95 Changed 15 months ago by egourgoulhon

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 follow-up: Changed 15 months ago by tscrim

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.

Last edited 15 months ago by tscrim (previous) (diff)

comment:105 Changed 15 months ago by git

  • Commit changed from 19caedb4d055887ad33ab2ab7b051d166bd58c90 to 3cd03a48d847e12745ed8c25b23f19db141c179a

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

3cd03a4Add methods lift() and retract() to ManifoldSubset; add method __eq__() in CoordChange

comment:106 in reply to: ↑ 104 Changed 15 months ago by egourgoulhon

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 15 months ago by egourgoulhon

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:

Hierarchy-1:
               AbstractSet
               /         \
     ManifoldSubset    TopologicalManifold
             |      ____/        |
             |     /             |
  OpenTopSubmanifold             |
     |                           |
     |     DifferentiableMixin   |
     |       /              \    |
  OpenDiffSubmanifold   DifferentiableManifold
     |                           |
     |      IntervalMixin        |
     |       /          \        |
  OpenSubinterval       OpenInterval
                             |
                         RealLine

Now, prior to the refactoring, the class hierarchy was

Hierarchy-2:

      ManifoldSubset
            |
     TopologicalManifold
            |
   DifferentiableManifold
            |
        OpenInterval
            |
        RealLine

It is clear that Hierarchy-2 is much simpler than Hierarchy-1: it involves much less classes and has no multiple heritage issues. Moreover, Hierarchy-2 reflects fully the mathematics: the real line R is the open interval (-oo, +oo), which is a 1-dimensional differentiable manifold, which is a 1-dimensional topological manifold, which is a subset of a topological manifold (itself). Within Hierarchy-2, 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 Hierarchy-2 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 Hierarchy-1, the attribute _manifold exists only for the subset classes. For motivating Hierarchy-1, you said that it avoids tests of the type self is self._manifold. Now, in Hierarchy-2, 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 Hierarchy-1? Given its complexity, as compared with Hierarchy-2, I am wondering whether it is worth continuing with it... Note that with Hierarchy-2, 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 Hierarchy-2 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 Hierarchy-1, 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 follow-up: Changed 15 months ago by tscrim

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 Hierachy-2, 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 Hierarchy-1 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 15 months ago by egourgoulhon

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 be self 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 Hierarchy-1, 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 Hierachy-2, 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 Hierarchy-1 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?

Five times, as mentioned above.

Also what is your thoughts on maintenance and extendability?

It seems to me that, in terms of maintenance, Hierarchy-2 is better, being simpler; it is also certainly better for somebody new entering into the code. On the contrary, if one regards extendability, Hierarchy-1 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 Hierarchy-1 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 Hierarchy-1. 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.

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

comment:110 Changed 15 months ago by egourgoulhon

Giving a second thought to this, another hierarchy that would preserve the distinction between whole manifolds and open subsets is

Hierarchy-3: 

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

Hierarchy-3 is simpler than Hierarchy-1 and does not require any mixin class. It is also easy to add a new structure, like complex manifolds. Hierarchy-3 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 Hierarchy-1, 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 Hierarchy-3, 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?

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

comment:111 Changed 15 months ago by egourgoulhon

Here is another proposal, which is a kind of mix between Hierarchy-1 and Hierarchy-3:

Hierarchy-4:

 AbstractAmbient           AbstractSet              AbstractSubset
   |  |  |                      |    \              /   |   |   |
   |  |  |                      |     ManifoldSubset    |   |   |
   |  |  |                      |                       |   |   |
   |  |  |           TopologicalManifoldOpenSet         |   |   |
   |  |   \               /     |       \              /    |   |
   |  |   TopologicalManifold   |     OpenTopSubmanifold    |   |
   |  |                         |                           |   |
   |  |            DifferentiableManifoldOpenSet            |   |
   |   \                  /     |        \                 /    |
   |   DifferentiableManifold   |        OpenDiffSubmanifold    |
   |                            |                               |
   |                     IntervalOpenSet                       /
    \____            ____/            \             __________/
         OpenInterval                OpenSubInterval
              |
          RealLine

Contrary to Hierarchy-1, it has no diamond problem and involves no mixin class. As in Hierarchy-3, the class AbstractAmbient implements only the methods union and intersection, but contrary to Hierarchy-3, the classes for ambient objects (TopologicalManifold, DifferentiableManifold, etc.) do no longer have the attribute _manifold. This attribute is present only in the sub-object classes OpenTopSubmanifold, OpenDiffSubmanifold and OpenSubinterval, as in Hierarchy-1. In Hierarchy-4, 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 follow-up: Changed 15 months ago by tscrim

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 Hierarchy-3,4 to be effective.

It does seem like perhaps Hierarchy-2 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 well-established code in Sage for which there is a different class for subobjects.

Ah hell, perhaps we should just revert back to Hierarchy-2 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 15 months ago by egourgoulhon

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 why AbstractSubset would have to be a mixin, both in this hierarchy and Hierarchy-3,4 to be effective.

Yes. Similarly AbstractAmbient in Hierarchy-4 has to be a mixin, so that only AbstractSet inheritates from Parent.

It does seem like perhaps Hierarchy-2 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 well-established 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 Hierarchy-2 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 Hierarchy-2. 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 15 months ago by git

  • Commit changed from 3cd03a48d847e12745ed8c25b23f19db141c179a to 984c3c26bf827f44eee26fc6afd321e11dca8f2e

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

984c3c2Revert to simple hierarchy for manifold classes

comment:115 Changed 15 months ago by egourgoulhon

  • Status changed from needs_info to needs_review

Following comment:113, the above commit reverts to Hierarchy-2 (only the first two items of it, ManifoldSubset and TopologicalManifold are implemented in this ticket).

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

comment:116 Changed 15 months ago by egourgoulhon

  • Description modified (diff)

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

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 15 months ago by egourgoulhon

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 14 months ago by git

  • Commit changed from 984c3c26bf827f44eee26fc6afd321e11dca8f2e to 4ed37cece6c44c8c55e79b4dd9990eafe29c20d9

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

af727a8Merge branch 'public/manifolds/top_manif_basics' of trac.sagemath.org:sage into public/manifolds/top_manif_basics
1113442Some final reviewer changes and tweaks.
4ed37ceSome documentation rewording and other small tweaks.

comment:120 follow-up: Changed 14 months ago by tscrim

  • Milestone changed from sage-7.0 to sage-7.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 keyword check that I changed to verbose 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 be False.
  • In ManifoldPoint, the methods set_coord should be the full set_coordinates (and similar methods). The short version can be an alias. (Same discussion as for disp vs display.).

Once we take care of these, then this will be a positive review.

comment:121 Changed 14 months ago by git

  • Commit changed from 4ed37cece6c44c8c55e79b4dd9990eafe29c20d9 to cf5e98b2041d1fdee0756e99a85ada0b865375da

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

cf5e98bUse standard copyright template.

comment:122 Changed 14 months ago by git

  • Commit changed from cf5e98b2041d1fdee0756e99a85ada0b865375da to 00d265cf2855121dba914868264da6ea3a9c42af

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

0c9d7faMerge branch 'public/manifolds/top_manif_basics' into Sage 7.1.beta0.
00d265cSet verbose=False as the default in CoordChange.set_inverse; change "coord" to "coordinates" in names of ManifoldPoint methods.

comment:123 in reply to: ↑ 120 Changed 14 months ago by egourgoulhon

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 keyword check that I changed to verbose 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 methods set_coord should be the full set_coordinates (and similar methods). The short version can be an alias. (Same discussion as for disp vs display.).

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 14 months ago by tscrim

  • 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 14 months ago by egourgoulhon

Thanks a lot for the detailed review work, the numerous improvements and the enlightening discussions!

comment:126 Changed 14 months ago by vbraun

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