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]

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]

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

[sagetrac-git]

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

[sagetrac-git]

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

[sagetrac-git]

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

600c646

Trac #30799: models -> catalog

[sagetrac-git]

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.

[sagetrac-git]

Changed commit from 600c646 to 56148ba

[sagetrac-git]

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

56148ba

Trac #30799: add new location to rst file

[sagetrac-git]

Changed commit from 56148ba to 2d49e3e

[sagetrac-git]

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.

[sagetrac-git]

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

7ad486d

Trac #30799: doc adapted

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

[tobiasdiez]

comment:41

Ok, I'm convinced: +1 for examples. I would also tend to put every example in its own file. It's more consistent and I don't see any disadvantages of having "small" classes.

For the pattern, it probably depends on the concrete example and how much the implementations differ. In general, the factory pattern is good when you have completely independent implementations of some interface (e.g. getting infos from a file vs from an web api) whereas the façade pattern excels at hiding the implementation details. You can also combine them easily.
For example, for the sphere one could imagine the following:

• A general class Sphere that acts as a façade for Sphere2Dim and SphereHigherDim (e.g. because you might have more effective ways to calculate in 2d).
• Classes for d=1,3,7 deriving from Sphere and adding the parallelizable aspect.
• Factory method in catalog.py that based on the dimension invokes the constructor of Sphere or one of its subclasses for d=1,3,7

[mjungmath]

comment:42

Replying to @tobiasdiez:

I would also tend to put every example in its own file. It's more consistent and I don't see any disadvantages of having "small" classes.

I disagree here. Dedicated methods to each example are a good thing anyway imho, see comment:40. So why not leave the simple constructions there?

For the pattern, it probably depends on the concrete example and how much the implementations differ. In general, the factory pattern is good when you have completely independent implementations of some interface (e.g. getting infos from a file vs from an web api) whereas the façade pattern excels at hiding the implementation details. You can also combine them easily.
For example, for the sphere one could imagine the following:

• A general class Sphere that acts as a façade for Sphere2Dim and SphereHigherDim (e.g. because you might have more effective ways to calculate in 2d).
• Classes for d=1,3,7 deriving from Sphere and adding the parallelizable aspect.
• Factory method in catalog.py that based on the dimension invokes the constructor of Sphere or one of its subclasses for d=1,3,7

Totally! I think this is how we should do it. The parallelizable cases have the same charts, but in contrast to their brothers a global frame which can be constructed in the corresponding classes. So, I'd prefer the façade pattern here as well. But this is something we should discuss in more detail in #30804.

[sagetrac-git]

Changed commit from 8d2f44d to 167fa25

[sagetrac-git]

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

6581bc9	Merge branch 'develop' into t/30799/add_folder_for_manifold_models
167fa25	Trac #30799: unique_tag for examples

[mjungmath]

comment:45

What about this? I think, the UniqueRepresentation behavior should now be consistent with Manifold().

However, with this commit, there is a doctest error in line 215 of real_line.py...

[mjungmath]

comment:46

Replying to @mjungmath:

However, with this commit, there is a doctest error in line 215 of real_line.py...

See #30830 for details.

[mjungmath]

comment:47

Replying to @mjungmath:

What about this? I think, the UniqueRepresentation behavior should now be consistent with Manifold().

However, with this commit, there is a doctest error in line 215 of real_line.py...

To elaborate: each manifold constructed from the global namespace, i.e. via the factory method, obtains a unique tag. That means, each invokation constructs a new object.

For consistency, this should hold for OpenInterval and RealLine, too. And for more consistency, it is good to manage this behavior via factory methods.

Please object if you disagree.

[egourgoulhon]

comment:48

Replying to @mjungmath:

Replying to @mjungmath:

What about this? I think, the UniqueRepresentation behavior should now be consistent with Manifold().

However, with this commit, there is a doctest error in line 215 of real_line.py...

To elaborate: each manifold constructed from the global namespace, i.e. via the factory method, obtains a unique tag. That means, each invokation constructs a new object.

For consistency, this should hold for OpenInterval and RealLine, too.

No, OpenInterval and RealLine are truely UniqueRepresentation objects. The unique tag trick shall not apply to them.

Anyway, the unique tag issue is beyond the scope of this ticket, I believe.
At some point, we should get rid of UniqueRepresentation inheritance for generic manifolds. It is a workaround to have a correct pickling, which is necessary for parallelized computations.

[mjungmath]

comment:49

Replying to @egourgoulhon:

No, OpenInterval and RealLine are truely UniqueRepresentation objects. The unique tag trick shall not apply to them.

What is the intention behind it?

Anyway, the unique tag issue is beyond the scope of this ticket, I believe.
At some point, we should get rid of UniqueRepresentation inheritance for generic manifolds. It is a workaround to have a correct pickling, which is necessary for parallelized computations.

You're right. However, it is somehow entwined. It wouldn't make sense to create a factory method somewhere else than in catalog.py.

[egourgoulhon]

comment:50

Replying to @mjungmath:

Replying to @egourgoulhon:

No, OpenInterval and RealLine are truely UniqueRepresentation objects. The unique tag trick shall not apply to them.

What is the intention behind it?

RealLine() or OpenInterval(0,1) fully defines the mathematical object, Manifold(2, 'M') does not.

[mjungmath]

comment:51

Replying to @egourgoulhon:

Replying to @mjungmath:

Replying to @egourgoulhon:

No, OpenInterval and RealLine are truely UniqueRepresentation objects. The unique tag trick shall not apply to them.

What is the intention behind it?

RealLine() or OpenInterval(0,1) fully defines the mathematical object, Manifold(2, 'M') does not.

Mh, I see. And EuclideanSpace(2) could still yield two different but isomorphic mathematical entities.

But what about OpenInterval(0,1) and OpenInterval(0,1, name='I'). Or OpenInterval(-oo,oo) and RealLine(). Same mathematical object, but different instances.

I think, this should be catched via __classcall_private__ then. However, that's definitely beyond the scope of this ticket.

---

So, what's your proposal with the import/method/catalog behavior then? Keep the version of comment:29?

[tscrim]

comment:52

Replying to @egourgoulhon:

Replying to @mjungmath:

Replying to @mjungmath:

What about this? I think, the UniqueRepresentation behavior should now be consistent with Manifold().

However, with this commit, there is a doctest error in line 215 of real_line.py...

To elaborate: each manifold constructed from the global namespace, i.e. via the factory method, obtains a unique tag. That means, each invokation constructs a new object.

For consistency, this should hold for OpenInterval and RealLine, too.

No, OpenInterval and RealLine are truely UniqueRepresentation objects. The unique tag trick shall not apply to them.

Anyway, the unique tag issue is beyond the scope of this ticket, I believe.
At some point, we should get rid of UniqueRepresentation inheritance for generic manifolds. It is a workaround to have a correct pickling, which is necessary for parallelized computations.

I still haven't forgotten that I promised you I would do that. It is on my to-do list. Sorry for taking so long.

[sagetrac-git]

Changed commit from 167fa25 to 8d2f44d

[sagetrac-git]

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

[mjungmath]

comment:54

Like Eric pointed out correctly, some things discussed here go beyond the scope of this ticket. I reverted the branch to an older version, where I used lazy imports into catalog.py.

Lazy import should prevent circular and unneccessary imports.

Patchbot is already green.

[tscrim]

comment:55

We can do further changes in other tickets. I think it is good to take some smaller steps in each ticket.

[tscrim]

Reviewer: Travis Scrimshaw

[mjungmath]

comment:56

I agree.

Thanks.

[vbraun]

Changed branch from u/gh-mjungmath/add_folder_for_manifold_models to 8d2f44d