Add Folder for Manifold Examples

[mjungmath]

Belongs to #30189.

I propose to add a new folder for manifold examples:

src/sage/manifolds/differentiable/examples

Furthermore, I suggest we move euclidean.py and real_line.py to that folder and make it, besides the global availability, accessible from the catalog, too. The idea is that any manifold example can be accessed via the catalog, and the corresponding files are stored in the examples folder.

For manifolds with non-smooth structure (or non-diffeomorphic structures), we can add such a folder in manifolds, too. But I don't see the need yet.

CC: @egourgoulhon @tscrim

Component: manifolds

Author: Michael Jung

Branch/Commit: 8d2f44d

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/30799

[mjungmath]

Description changed:

--- 
+++ 
@@ -2,6 +2,6 @@

 `src/sage/manifolds/differentiable/models`

-Furthermore, I suggest we move `EuclideanSpace` to that folder and make it, besides the global availability, accessible from the catalog, too. The idea is that any manifold model can be accessed via the catalog, and the corresponding files are stored in the `model` folder.
+Furthermore, I suggest we move `EuclideanSpace` to that folder and make it, besides the global availability, accessible from the catalog, too. The idea is that any manifold model can be accessed via the catalog, and the corresponding files are stored in the `models` folder.

 For manifolds with non-smooth structure (or non-diffeomorphic structures), we can add such a folder in `manifolds`, too. But I don't see the need yet.

[mjungmath]

comment:1

There are now two way that I see (not related to this ticket directly):

1) The methods in catalog.py operate as a factory method for each model.

2) Each model manages its own factory in its corresponding file (as in EuclideanSpace right now).

Namely, some dimensions have peculiarities that we may want to consider (e.g. S³ is parallelizable).

[mjungmath]

Description changed:

--- 
+++ 
@@ -1,3 +1,5 @@
+Belongs to #30189.
+
 I propose to add a new folder for manifold models:

 `src/sage/manifolds/differentiable/models`

[mjungmath]

Description changed:

--- 
+++ 
@@ -4,6 +4,6 @@

 `src/sage/manifolds/differentiable/models`

-Furthermore, I suggest we move `EuclideanSpace` to that folder and make it, besides the global availability, accessible from the catalog, too. The idea is that any manifold model can be accessed via the catalog, and the corresponding files are stored in the `models` folder.
+Furthermore, I suggest we move `euclidean.py` and `real_line.py` to that folder and make it, besides the global availability, accessible from the catalog, too. The idea is that any manifold model can be accessed via the catalog, and the corresponding files are stored in the `models` folder.

 For manifolds with non-smooth structure (or non-diffeomorphic structures), we can add such a folder in `manifolds`, too. But I don't see the need yet.

[mjungmath]

Branch: u/gh-mjungmath/add_folder_for_manifold_models

[mjungmath]

comment:5

Here's my proposal in concrete form. Feedback is welcome.

---

New commits:

2cbfd30

Trac #30799: collect manifold models in 'model' module

[mjungmath]

Commit: 2cbfd30

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

96d4382

Trac #30799: collect manifold models in 'models' module

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 2cbfd30 to 96d4382

[mjungmath]

comment:7

I struggle a bit with the word "model". Other ideas are welcome.

[tscrim]

comment:8

I don't care for the name "model" as it means something slightly different to me than what you intend. I would say "examples" is a better name.

[mjungmath]

comment:9

What about calling the folder simply catalog? Or would that mess up the structure with the catalog.py in the manifold's main folder?

[egourgoulhon]

comment:10

Thanks for this ticket. A folder dedicated to standard manifolds sounds like a good idea. Regarding the name, I would lean to catalog as suggested in comment:9, with an import statement of the form

from sage.manifolds.catalog import Torus   

instead of

from sage.manifolds.differentiable.catalog import Torus   

which looks unnecessarily longer and probably more difficult to find via introspection.

Apparently, there is no catalog folder in all src/sage, only catalog.py files. I don't know if there is any reason for this...

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

600c646

Trac #30799: models -> catalog

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 96d4382 to 600c646

[mjungmath]

Author: Michael Jung

[mjungmath]

comment:12

Doctest works out for manifolds folder. I hope I catched all docstrings refering to EuclideanSpace and real_line.

[mjungmath]

Description changed:

--- 
+++ 
@@ -2,8 +2,8 @@

 I propose to add a new folder for manifold models:

-`src/sage/manifolds/differentiable/models`
+`src/sage/manifolds/differentiable/catalog`

-Furthermore, I suggest we move `euclidean.py` and `real_line.py` to that folder and make it, besides the global availability, accessible from the catalog, too. The idea is that any manifold model can be accessed via the catalog, and the corresponding files are stored in the `models` folder.
+Furthermore, I suggest we move `euclidean.py` and `real_line.py` to that folder and make it, besides the global availability, accessible from the catalog, too. The idea is that any manifold model can be accessed via the catalog, and the corresponding files are stored in the `catalog` folder.

 For manifolds with non-smooth structure (or non-diffeomorphic structures), we can add such a folder in `manifolds`, too. But I don't see the need yet.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 600c646 to 56148ba

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

56148ba

Trac #30799: add new location to rst file

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 56148ba to 2d49e3e

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

2d49e3e

Trac #30799: imports and docstrings adapted

[tscrim]

comment:16

Usually there is just a catalog.py because things are not sufficiently complex to warrant a separate subfolder and there can be simple(-ish) functions that create objects that benefit from being together in one file.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

7ad486d

Trac #30799: doc adapted

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 2d49e3e to 7ad486d

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 7ad486d to 35afb82

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

35afb82

Trac #30799: doc and imports adapted

[mjungmath]

comment:20

Patchbot is green now. :)

[mjungmath]

comment:21

Replying to @egourgoulhon:

Thanks for this ticket. A folder dedicated to standard manifolds sounds like a good idea. Regarding the name, I would lean to catalog as suggested in comment:9, with an import statement of the form

from sage.manifolds.catalog import Torus   

instead of

from sage.manifolds.differentiable.catalog import Torus   

which looks unnecessarily longer and probably more difficult to find via introspection.

Apparently, there is no catalog folder in all src/sage, only catalog.py files. I don't know if there is any reason for this...

Btw, in addition to that, the examples can currently be invoked by

manifolds.Torus()

This comes from the fact that all.py imports

import sage.manifolds.catalog as manifolds

I think this is very nice, too.

[tobiasdiez]

comment:22

I would suggest to import the different models in the catalog/__init__.py file, so that from a user-perspective there is no real difference in having a big catalog.py file or a catalog module. This would simplify the import

from sage.manifolds.differentiable.catalog.real_line import RealLine

to

from sage.manifolds.differentiable.catalog import RealLine

[mjungmath]

comment:23

Replying to @tobiasdiez:

I would suggest to import the different models in the catalog/__init__.py file, so that from a user-perspective there is no real difference in having a big catalog.py file or a catalog module. This would simplify the import

from sage.manifolds.differentiable.catalog.real_line import RealLine

to

from sage.manifolds.differentiable.catalog import RealLine

The import pattern is currently

S2 = manifolds.Sphere(2)

and

from sage.manifolds.catalog import Sphere

S2 = Sphere(2)

I think this is what we should promote. As Eric pointed out in comment:10,

from sage.manifolds.differentiable.catalog...

is something that should not be promoted, and I agree with him.

However, even if the import pattern is the same, the suggested folder structure is nice because it indicates what examples are differentiable manifolds and what not.

[tobiasdiez]

comment:24

Ok, I also like these conventions. Then I would also promote them in the "normal" Sage code, e.g. from sage.manifolds.catalog import EuclideanSpace instead of from sage.manifolds.differentiable.catalog.euclidean import EuclideanSpace (in the changes in catalog.py).

[mjungmath]

comment:25

Replying to @tobiasdiez:

Ok, I also like these conventions. Then I would also promote them in the "normal" Sage code, e.g. from sage.manifolds.catalog import EuclideanSpace instead of from sage.manifolds.differentiable.catalog.euclidean import EuclideanSpace (in the changes in catalog.py).

Then I see the problem that the imports rely on the catalog import pattern too much. If we decide to change it at some point, there is too much dependency involved. I personally don't care when the internal import statements are that long.

I think it is always good to import the modules internally from where they are. Besides, I can imagine that the work around could cause a slight slowdown, because the catalog module contains further code which is not necessary for the EuclideanSpace.

Eric, Travis? What do you say?

[mjungmath]

comment:26

The more I think about it, the more I tend to an examples folder instead. This is at least consistent with other modules (see e.g. homology).

[egourgoulhon]

comment:27

Replying to @mjungmath:

Btw, in addition to that, the examples can currently be invoked by

manifolds.Torus()

This comes from the fact that all.py imports

import sage.manifolds.catalog as manifolds

I think this is very nice, too.

Ah yes indeed, that's the standard way to use the catalog. Much better than the import statement I mentioned in comment:10. So let us forget about the latter.

[egourgoulhon]

comment:28

Replying to @mjungmath:

Replying to @tobiasdiez:

Ok, I also like these conventions. Then I would also promote them in the "normal" Sage code, e.g. from sage.manifolds.catalog import EuclideanSpace instead of from sage.manifolds.differentiable.catalog.euclidean import EuclideanSpace (in the changes in catalog.py).

Then I see the problem that the imports rely on the catalog import pattern too much. If we decide to change it at some point, there is too much dependency involved. I personally don't care when the internal import statements are that long.

+1

I think it is always good to import the modules internally from where they are.

Indeed.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 35afb82 to 8d2f44d

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

8d2f44d

Trac #30799: collect manifolds examples in 'examples' + lazy_import for catalog

[mjungmath]

Description changed:

--- 
+++ 
@@ -1,9 +1,9 @@
 Belongs to #30189.

-I propose to add a new folder for manifold models:
+I propose to add a new folder for manifold examples:

-`src/sage/manifolds/differentiable/catalog`
+`src/sage/manifolds/differentiable/examples`

-Furthermore, I suggest we move `euclidean.py` and `real_line.py` to that folder and make it, besides the global availability, accessible from the catalog, too. The idea is that any manifold model can be accessed via the catalog, and the corresponding files are stored in the `catalog` folder.
+Furthermore, I suggest we move `euclidean.py` and `real_line.py` to that folder and make it, besides the global availability, accessible from the catalog, too. The idea is that any manifold example can be accessed via the catalog, and the corresponding files are stored in the `examples` folder.

 For manifolds with non-smooth structure (or non-diffeomorphic structures), we can add such a folder in `manifolds`, too. But I don't see the need yet.

[mjungmath]

comment:30

Changed catalog to examples and some further details. Wait for patchbot.

[egourgoulhon]

comment:31

I am not sure examples is a better name than catalog in the present case. Indeed, this folder shall contain most (all?) "standard" manifolds in low dimension: Euclidian spaces, spheres, tori, projective spaces, etc. It looks more than a mere collection of examples to me.

[mjungmath]

comment:32

Replying to @egourgoulhon:

I am not sure examples is a better name than catalog in the present case. Indeed, this folder shall contain most (all?) "standard" manifolds in low dimension: Euclidian spaces, spheres, tori, projective spaces, etc. It looks more than a mere collection of examples to me.

The homology module for instance calls their catalog members "examples", too. Moreover, I would prefer examples because it doesn't pollute the namespace too much with the preexisting catalog.py.

[tscrim]

comment:33

I would also go for examples rather than catalog because we can add functions to the catalog file that are not sufficiently big/detailed to require their own file.

Also, I strongly support directly important from the files at hand. This makes it a little easier to see where cyclic imports are coming from and to help prevent that.

[tobiasdiez]

comment:34

I vote for catalog. I thought the long-term goal was to replace the catalog.py file by the catalog module. You can then still easily add functions to catalog/__init__py.

[mjungmath]

comment:35

Stalemate! :D

Replying to @tobiasdiez:

I vote for catalog. I thought the long-term goal was to replace the catalog.py file by the catalog module.

Not precisely. As Travis pointed out in comment:33, the catalog.py still contains entire code of "small" examples. Besides, I think it is best if the catalog.py also manages the factories to construct the examples at hand. Let me give two arguments for that:

1. For example, inheriting from a Manifold class makes that class a UniqueRepresentation. But that is not the behavior we want for examples constructed from catalog.py (compare EuclideanSpace or the Manifold class and the Manifold factory method imported to the global namespace).

2. The factory method can delegate special dimensions, exotic counterparts or different realizations to the corresponding implementation.

You can then still easily add functions to catalog/__init__py.

I would advice against it. Nobody expects code in __init__.py.

[tscrim]

comment:36

Replying to @tobiasdiez:

I vote for catalog. I thought the long-term goal was to replace the catalog.py file by the catalog module. You can then still easily add functions to catalog/__init__py.

That doesn't quite make sense, catalog.py is a module (in Python-speak). If you mean folder, then no, that is not the long-term goal.

However, there is not any specific reason to not have code in __init__.py.

[tobiasdiez]

comment:37

Ok, what is then the long-term goal?

• The catalog.py contains a few examples. But all of them are differentiable and seem complex enough to be put in a separate classes. Should they be put in separate files in the folder manifolds/differentiable/catalog?
• Where should one add a example that is not differentiable? The folder manifolds/catalog would be the natural choice. But having such a folder would lead to problems as packages have priority over modules, so the catalog.py module would not be visible. That's why I proposed to replace the catalog module (file) by the catalog package (directory).

---

The factory method can delegate special dimensions, exotic counterparts or different realizations to the corresponding implementation.

I would solve this using the façade pattern. So, if dimension 2 would be special for the sphere, you have a class Sphere for every dimension that delegates its work to Sphere2Dim and SphereHigherDim where the actual implementation is taking place.

[mjungmath]

comment:38

Replying to @tobiasdiez:

The catalog.py contains a few examples. But all of them are differentiable and seem complex enough to be put in a separate classes. Should they be put in separate files in the folder manifolds/differentiable/catalog?

You got a point. The most simple examples, Euclidean space and open intervals, are already complex enough to occupy whole files. On the other hand, some examples might stay primitive and are too short to occupy an own file.

Where should one add a example that is not differentiable? The folder manifolds/catalog would be the natural choice. But having such a folder would lead to problems as packages have priority over modules, so the catalog.py module would not be visible. That's why I proposed to replace the catalog module (file) by the catalog package (directory).

This was exactly my worry and is the reason why I proposed examples as folder name. If we do as you propose, we additionally have to make sure that the documentation about the catalog remains at least somewhere; __init__.py is no option as far as I can see.

I would solve this using the façade pattern. So, if dimension 2 would be special for the sphere, you have a class Sphere for every dimension that delegates its work to Sphere2Dim and SphereHigherDim where the actual implementation is taking place.

This sounds like a reasonable approach, too. It is also consistent with the current implementation of EuclideanSpace.

On the other hand, the only thing I'm unsure about is whether this approach is flexible enough for adding new examples/alternatives step-by-step, also by developers who might not be not familiar with our discussion here. As for the factory method this is easy and comprehensible: just add another if-clause and/or option.

[egourgoulhon]

comment:39

Replying to @mjungmath:

Replying to @tobiasdiez:

The catalog.py contains a few examples. But all of them are differentiable and seem complex enough to be put in a separate classes. Should they be put in separate files in the folder manifolds/differentiable/catalog?

You got a point. The most simple examples, Euclidean space and open intervals, are already complex enough to occupy whole files. On the other hand, some examples might stay primitive and are too short to occupy an own file.

Indeed.

Where should one add a example that is not differentiable? The folder manifolds/catalog would be the natural choice. But having such a folder would lead to problems as packages have priority over modules, so the catalog.py module would not be visible. That's why I proposed to replace the catalog module (file) by the catalog package (directory).

This was exactly my worry and is the reason why I proposed examples as folder name. If we do as you propose, we additionally have to make sure that the documentation about the catalog remains at least somewhere; __init__.py is no option as far as I can see.

OK, to be consistent with the rest of Sage code, it's probably wise to keep the catalog.py file and rename the folder(s) by something else than catalog. I was somewhat reluctant about examples, but at the moment I don't have any better name to suggest (besides models as already suggested by Michael, realizations came to my mind but I am not sure it's pertinent...).

I would solve this using the façade pattern. So, if dimension 2 would be special for the sphere, you have a class Sphere for every dimension that delegates its work to Sphere2Dim and SphereHigherDim where the actual implementation is taking place.

This sounds like a reasonable approach, too. It is also consistent with the current implementation of EuclideanSpace.

Yes. Most probably, we need a Sphere2D class, at least for the standard naming of spherical coordinates as (theta, phi). We shall also need special subclasses for parallelizable spheres (S¹, S³ and S⁷).

On the other hand, the only thing I'm unsure about is whether this approach is flexible enough for adding new examples/alternatives step-by-step, also by developers who might not be not familiar with our discussion here. As for the factory method this is easy and comprehensible: just add another if-clause and/or option.

Yes factory approach might be better in this respect. It avoids to invoke __classcall_private__, as currently done in EuclideanSpace.

[mjungmath]

comment:40

Allow me to summarize the accumulated discussion. Unfortunately, we can't start polls here (a feature I completely miss).

• For consistency, to keep the documentation, and to obtain a more comprehensible factory pattern, it is better to keep the catalog.py.
• In conclusion, we have to choose another name for the folders. Possible suggestions:
  1. models
  2. examples
  3. realizations
• Which pattern?
  1. Special dimensions of the same implementation could be treated via facade pattern as it has been done for EuclideanSpace, whereas more deviating realizations could be treated via factory method pattern in catalog.py.
  2. Everything is regulated via facade.
  3. Everything is regulated via factory method in catalog.py.

---

With respect to the last point, I suggest that every cataloged manifold should have it's own method in catalog.py for documentation and unification reasons. This also avoids duplicated or circular imports.

---

Please let me know if I caught something wrong.