Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invoke manifolds with structure more conveniently #33001

Closed
mjungmath opened this issue Dec 9, 2021 · 32 comments
Closed

Invoke manifolds with structure more conveniently #33001

mjungmath opened this issue Dec 9, 2021 · 32 comments

Comments

@mjungmath
Copy link

Inspired by the discussion in #30362, comment:77, we change the Manifold constructor so that once can omit the structure parameter when parameters such as diff_degree or metric_name are given:

        sage: M = Manifold(3, 'M', diff_degree=0); M
        3-dimensional topological manifold M
        sage: M = Manifold(3, 'M', diff_degree=2); M
        3-dimensional differentiable manifold M
        sage: M = Manifold(3, 'M', metric_name='g'); M
        3-dimensional Riemannian manifold M

CC: @egourgoulhon @tscrim @tobiasdiez

Component: manifolds

Author: Matthias Koeppe

Branch/Commit: 9548b50

Reviewer: Travis Scrimshaw

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

@mjungmath mjungmath added this to the sage-9.5 milestone Dec 9, 2021
@mkoeppe
Copy link
Contributor

mkoeppe commented Dec 9, 2021

comment:1

-1 on adding classes such as TopologicalManifold to manifolds; to my understanding manifolds, like polytopes, groups, algebras, is intended to provide a "catalog" of examples, not a general "namespace".

@mjungmath
Copy link
Author

comment:2

Alright, I see. That makes sense. What about something like Manifold.Topological(...)?

@mkoeppe
Copy link
Contributor

mkoeppe commented Dec 9, 2021

comment:3

I'd rather suggest to look into having certain keyword arguments trigger a choice of structure.

sage: Manifold(2, 'M', diff_degree=7)
ValueError: diff_degree = 7 is not compatible with a smooth structure

Instead, this could perhaps imply structure='differentiable'.

sage: Manifold(2, 'M', metric_name='g')
2-dimensional differentiable manifold M

Instead, this should probably select structure='Riemannian'.

It would also be good if users could pass a category keyword to select the structure. In this direction, also for example the Riemannian structure is not yet reflected in the categories:

sage: E = EuclideanSpace(2)
sage: E.category()
Join of Category of smooth manifolds over Real Field with 53 bits of precision and Category of connected manifolds over Real Field with 53 bits of precision and Category of complete metric spaces

This is something that we should extend along the way.

@tscrim
Copy link
Collaborator

tscrim commented Dec 9, 2021

comment:4

I agree with Matthias. I would not be opposed to manifolds.Topological, but I like having default behavior depend on extra parameters. +1 on adding more categories (although we have to differentiate be metric_function and metric_tensor) and having the category information yield the structure. Of course, in all cases, if the user explicitly passes structure, that should take priority or raise an error if there is a conflict.

@mjungmath
Copy link
Author

comment:5

I agree. The categorical approach is the most natural one.

I think we should get rid of the structure implementation as it is right now and entirely jump over to categories. This would also have the great advantage that we can combine structures such as symplectic and Riemannian structures, which are not mutually exclusive.

Also this default behavior on extra parameters is a good idea.

@tscrim
Copy link
Collaborator

tscrim commented Dec 10, 2021

comment:6

I am fairly -1 on pushing the full structure into the categories. This was a very conscious design decision in full view of what could be done with categories.

  1. It makes it difficult to easily change. Right now, it is trivial if someone wants to promote/demote the structure of a manifold. (You change a parameter and do not have to create an entire new object, which is relatively expensive for manifolds.)
  2. It allows for something easy to transfer information across different base rings.
  3. It is easy to have extra parameterizations. For example, the classes of Ck manifolds.

It is meant to work in conjunction with the categories.

@mjungmath
Copy link
Author

comment:7

Replying to @tscrim:

I am fairly -1 on pushing the full structure into the categories. This was a very conscious design decision in full view of what could be done with categories.

  1. It makes it difficult to easily change. Right now, it is trivial if someone wants to promote/demote the structure of a manifold. (You change a parameter and do not have to create an entire new object, which is relatively expensive for manifolds.)
  2. It allows for something easy to transfer information across different base rings.
  3. It is easy to have extra parameterizations. For example, the classes of Ck manifolds.

It is meant to work in conjunction with the categories.

Can you elaborate on 1, please?

As for 2, do you have an example where the structure design makes things easier opposed to categories?

I see the point in 3.

Nevertheless, we should rethink the design in some way. We have Riemannian manifolds, which are represented by the class PseudoRiemannianManifold and carry the structure RiemannianStructure. With #30362 we get new structures, namely Poisson/symplectic structures. But now, a manifold can have a Riemannian structure and a symplectic structure. Which base class do we use? Which structure? Do we want to combine structures? If so, how? I just see that the category framework already comes with the benefit of joint categories. It might be easier to use that preexisting architecture.

@tscrim
Copy link
Collaborator

tscrim commented Dec 11, 2021

comment:8

Replying to @mjungmath:

Replying to @tscrim:

I am fairly -1 on pushing the full structure into the categories. This was a very conscious design decision in full view of what could be done with categories.

  1. It makes it difficult to easily change. Right now, it is trivial if someone wants to promote/demote the structure of a manifold. (You change a parameter and do not have to create an entire new object, which is relatively expensive for manifolds.)
  2. It allows for something easy to transfer information across different base rings.
  3. It is easy to have extra parameterizations. For example, the classes of Ck manifolds.

It is meant to work in conjunction with the categories.

Can you elaborate on 1, please?

If you have a manifold M with structure S, and you realize that it should have structure T. This becomes a very simple change of swapping S for T. In particular, you want to change the smoothness. (Admittedly, I don't think we have a good hook for doing this.)

As for 2, do you have an example where the structure design makes things easier opposed to categories?

You want information preserved for manifolds over CC and over RR. These will lie in different categories. It also gets around not having a clear specification of RR (e.g., no dependency on precision or the particular implementation). It is also better to identify information with attributes rather than methods (e.g., the chart class) for speed and readability.

Nevertheless, we should rethink the design in some way. We have Riemannian manifolds, which are represented by the class PseudoRiemannianManifold and carry the structure RiemannianStructure. With #30362 we get new structures, namely Poisson/symplectic structures. But now, a manifold can have a Riemannian structure and a symplectic structure. Which base class do we use? Which structure? Do we want to combine structures? If so, how? I just see that the category framework already comes with the benefit of joint categories. It might be easier to use that preexisting architecture.

Don't conflate structures and base classes. They are different and serve different roles. Don't try and force a square peg into a round hole. Right now there is not a combinatorial explosion of cases, so we don't need to use a backhoe when all we need is a shovel either. If you need a combined structure, then make a combined structure.

@mjungmath
Copy link
Author

comment:9

Replying to @tscrim:

If you have a manifold M with structure S, and you realize that it should have structure T. This becomes a very simple change of swapping S for T. In particular, you want to change the smoothness. (Admittedly, I don't think we have a good hook for doing this.)

Why can't you just swap the category accordingly? As far as I see it, this is the same concept.

You want information preserved for manifolds over CC and over RR. These will lie in different categories. It also gets around not having a clear specification of RR (e.g., no dependency on precision or the particular implementation). It is also better to identify information with attributes rather than methods (e.g., the chart class) for speed and readability.

That's a good argument. I think, though, this can easily be circumvented by using a category base class that is not build upon a "base ring" structure.

Don't conflate structures and base classes. They are different and serve different roles. Don't try and force a square peg into a round hole. Right now there is not a combinatorial explosion of cases, so we don't need to use a backhoe when all we need is a shovel either. If you need a combined structure, then make a combined structure.

Well, that is my point. Base classes and structures are different, and yet we have a Riemannian manifold base class and structure.

Though I'd prefer categories; if we stay with the structure design, I'd feel more comfortable if we'd implement joins of structures. This is far more flexible and robust for future structures. It's getting harder to change the architecture when the "combinatorial explosion of cases" is already too high.

@tscrim
Copy link
Collaborator

tscrim commented Dec 11, 2021

comment:10

Replying to @mjungmath:

Replying to @tscrim:

If you have a manifold M with structure S, and you realize that it should have structure T. This becomes a very simple change of swapping S for T. In particular, you want to change the smoothness. (Admittedly, I don't think we have a good hook for doing this.)

Why can't you just swap the category accordingly? As far as I see it, this is the same concept.

Because categories are strongly tied to the creation of the object (and its elements) as it creates a dynamic subclass. In fact, strictly speaking, the differentiable structure of a manifold can give different objects in the category of differentiable manifolds (although we have actually pushed this data into the manifold itself).

You want information preserved for manifolds over CC and over RR. These will lie in different categories. It also gets around not having a clear specification of RR (e.g., no dependency on precision or the particular implementation). It is also better to identify information with attributes rather than methods (e.g., the chart class) for speed and readability.

That's a good argument. I think, though, this can easily be circumvented by using a category base class that is not build upon a "base ring" structure.

Categories carry mathematical information. You cannot separate the base ring.

Don't conflate structures and base classes. They are different and serve different roles. Don't try and force a square peg into a round hole. Right now there is not a combinatorial explosion of cases, so we don't need to use a backhoe when all we need is a shovel either. If you need a combined structure, then make a combined structure.

Well, that is my point. Base classes and structures are different, and yet we have a Riemannian manifold base class and structure.

You cannot get around having a base class when you want extra methods, at least without a performance penalty or monkey-patching. However, it can have a range of structures attached to it.

Though I'd prefer categories; if we stay with the structure design, I'd feel more comfortable if we'd implement joins of structures. This is far more flexible and robust for future structures. It's getting harder to change the architecture when the "combinatorial explosion of cases" is already too high.

That is a fallacy as good OOP design means you do not care about how the backend is implemented. This sounds like the engineer's paradox: spend 30% more time making something 10% faster.

@tobiasdiez
Copy link
Contributor

comment:11

My remark originally was that I find TopologicalManifold('M') easier to write and remember than Manifold('M', structure='topological'). What about exposing the different classes in mainfold/all?
At least for a start the initialization code such as
https://github.com/sagemath/sage-prod/blob/develop/src/sage/manifolds/manifold.py#L2960-L2970
should go in the corresponding constructor of Topological(Sub)Manifold in my opinion so that Manifold is really only choosing the proper class to initialize based on the parameters provided. Then one can choose if one prefers to import TopologicalManifold and uses it directly, or is the Manifold constructor.

I cannot really contribute to the category vs structure discussion as I'm not familiar enough with the category framework. (If someone wants to enlighten me: what are the practical consequences of viewing \C as a complex manifold vs as a real manifold?)
From a mathematical perspective, the 'structure' is often easy to define whereas the relevant category may depend on the applications one has in mind (e.g. https://en.wikipedia.org/wiki/Symplectic_category).

@mkoeppe
Copy link
Contributor

mkoeppe commented Dec 12, 2021

@mkoeppe
Copy link
Contributor

mkoeppe commented Dec 12, 2021

comment:13

Replying to @mkoeppe:

I'd rather suggest to look into having certain keyword arguments trigger a choice of structure.

Implemented now


New commits:

6cc6200Manifold: Set default for 'structure' parameter from other keywords

@mkoeppe
Copy link
Contributor

mkoeppe commented Dec 12, 2021

Commit: 6cc6200

@mkoeppe
Copy link
Contributor

mkoeppe commented Dec 12, 2021

Author: Matthias Koeppe

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 13, 2021

Changed commit from 6cc6200 to fbb5ec6

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 13, 2021

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

fbb5ec6TopologicalSubmanifold._init_immersion: Move dimension check here from Manifold constructor

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 13, 2021

Changed commit from fbb5ec6 to 14fa055

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 13, 2021

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

14fa055Manifold: Move type checking for ambient to `__init__` methods

@mkoeppe
Copy link
Contributor

mkoeppe commented Dec 13, 2021

comment:17

Replying to @tobiasdiez:

for a start the initialization code such as
https://github.com/sagemath/sage-prod/blob/develop/src/sage/manifolds/manifold.py#L2960-L2970
should go in the corresponding constructor of Topological(Sub)Manifold in my opinion so that Manifold is really only choosing the proper class to initialize based on the parameters provided.

Done

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 13, 2021

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

9548b50TopologicalManifold.__init__: Move input checking for dimension parameter here from Manifold constructor

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 13, 2021

Changed commit from 14fa055 to 9548b50

@mkoeppe

This comment has been minimized.

@tscrim
Copy link
Collaborator

tscrim commented Dec 13, 2021

comment:20

Replying to @tobiasdiez:

My remark originally was that I find TopologicalManifold('M') easier to write and remember than Manifold('M', structure='topological'). What about exposing the different classes in mainfold/all?
At least for a start the initialization code such as
https://github.com/sagemath/sage-prod/blob/develop/src/sage/manifolds/manifold.py#L2960-L2970
should go in the corresponding constructor of Topological(Sub)Manifold in my opinion so that Manifold is really only choosing the proper class to initialize based on the parameters provided. Then one can choose if one prefers to import TopologicalManifold and uses it directly, or is the Manifold constructor.

I am very strong -1 on adding a bunch of things that pollute the global namespace when there is a generic concept (here a "manifold") and a global entry point. Not only does it create more imports at startup (even if done lazily, it still means more objects), more maintenance if names change, and more documentation fragmentation (it makes it much easier to decide where to put, e.g., construction information). (If you find it easier, then you can add it to your init.sage file.) We also cannot fully move that code to the initialization because at that point it is too late to decide whether or not we are a submanifold (or decide our structure). The safety checks could be moved if desired.

@mkoeppe
Copy link
Contributor

mkoeppe commented Dec 13, 2021

comment:21

The dispatching based on ambient could of course also be done in a __classcall__ method, but I haven't made this change on the branch.

@egourgoulhon
Copy link
Member

comment:22

Replying to @tscrim:

I am very strong -1 on adding a bunch of things that pollute the global namespace when there is a generic concept (here a "manifold") and a global entry point. Not only does it create more imports at startup (even if done lazily, it still means more objects), more maintenance if names change, and more documentation fragmentation (it makes it much easier to decide where to put, e.g., construction information). (If you find it easier, then you can add it to your init.sage file.) We also cannot fully move that code to the initialization because at that point it is too late to decide whether or not we are a submanifold (or decide our structure). The safety checks could be moved if desired.

I fully agree with all the above points.

@tobiasdiez
Copy link
Contributor

comment:23

Replying to @tscrim:

I am very strong -1 on adding a bunch of things that pollute the global namespace when there is a generic concept (here a "manifold") and a global entry point.

Fair enough!

We also cannot fully move that code to the initialization because at that point it is too late to decide whether or not we are a submanifold (or decide our structure).

I agree with the submanifold part, but why is it not possible to decide about the structure? It seems to me that the structure of a topological manifold is completely determined by the base field (and similarly for diff manifolds). One could probably even remove the structure constructor argument of TopologicalManifold, as its currently only used by the Manifold constructor and by subclasses. The latter can also directly set the variable _structure as they see fit. Has the advantage to make the whole structure thing an implementation detail that users of the class don't need to care about.

@mkoeppe mkoeppe modified the milestones: sage-9.5, sage-9.6 Dec 21, 2021
@mkoeppe

This comment has been minimized.

@tscrim
Copy link
Collaborator

tscrim commented Feb 21, 2022

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Feb 21, 2022

comment:26

LGTM.

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 21, 2022

comment:27

Thanks!

@vbraun
Copy link
Member

vbraun commented Feb 27, 2022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants