Replace manifold structures by inheritance

[tobiasdiez]

We remove the constructor argument structure and the private field _structure in the manifold class (and its subclasses). Instead, now inheritance is used to provide the information that originally were controlled by the structure. That is, the subclasses control now this data by either specifying it in the constructor (e.g. _name_modifer variable that be one of 'topological', 'differentiable', etc) or by overwriting methods (e.g. scalar_field_algebra).

Currently, the structure combines two aspects. Namely on one hand information that is in the definition of the manifold (e.g. if charts are real-valued) and on the other hand data that a manifold may carry in addition (e.g. Riemannian metric).
This leads to a bit of code duplication and potentially strange behavior.
For example, one can construct a topological manifold (using the TopologicalManifold constructor) that has the "structure" of a differentiable manifold. Another example is that one has different "structures" of Lorentzian and (pseudo-)Riemannian metrics, but in the end there is no intrinsic difference there except for the signature. In particular, these are all just differentiable manifolds at the core (and thus have the same algebra of smooth functions etc).

Based on the discussion in #33001.

Discussion points:

• Currently, a complex manifold is displayed as "1-dimensional complex manifold M" if its differentiable and as "Complex 1-dimensional topological manifold M" if its only topological. I find this somewhat confusing. What do you think about "Complex 3-dimensional differentiable/topological manifold M"?
• Should the display text be really "degenerate_metric manifold M" with underscore?

CC: @egourgoulhon @tscrim @mjungmath

Component: manifolds

Author: Tobias Diez

Branch/Commit: public/manifold/remove_structure @ 1231cc0

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

[tobiasdiez]

Changed branch from public/manifolds/remove_structure to public/manifold/remove_structure

[tobiasdiez]

Commit: 9bc4cb1

[tobiasdiez]

New commits:

f349dc6	Replace manifold structures by inheritance
1b51cb0	Merge branch 'develop' of https://github.com/sagemath/sage into public/manifold/remove_structure
9bc4cb1	Fix a few tests

[tobiasdiez]

comment:2

Apart from the discussion points listed in the ticket description and a few failing doctests this is awaiting feedback.

[mjungmath]

comment:3

To be honest, I don't understand the purpose of this ticket. Didn't we agree in #33001 to keep the structure implementation and set some keywords argument triggers instead?

---

Why do you change this?

             # trivial cases
             if self == 1:
-                self._mixed_forms[nab] = A(dom._one_scalar_field)
+                self._mixed_forms[nab] = A(dom.scalar_field_algebra().one())
             elif self == 0:
-                self._mixed_forms[nab] = A(dom._zero_scalar_field)
+                self._mixed_forms[nab] = A(dom.scalar_field_algebra().zero())

That was done on purpose. Besides, that is not part of this ticket.

Another thing I noticed already in other tickets:

         for dom in domain.open_supersets():
-            dom._zero_scalar_field._express[self] = self.function_ring().zero()
-            dom._one_scalar_field._express[self] = self.function_ring().one()
+            dom.scalar_field_algebra().zero()._express[
+                self
+            ] = self.function_ring().zero()
+            dom.scalar_field_algebra().one()._express[self] = self.function_ring().one()

Imho, line breaks like this make the code much harder to read.

In general, I would appreciate if you stick to the conventions that are already in the code. Switching back and forth between different conventions within one module (or file) just leaves mess.

In particular, I don't like that you add typing for just some parts. I'd prefer if we add typing for everything at once or leave things as they are (for now), but not some kind of hybrid.

[mjungmath]

comment:4

As for your both discussion points: I agree with you. Sounds good.

[tobiasdiez]

comment:5

Thanks for the quick feedback.

Replying to @mjungmath:

To be honest, I don't understand the purpose of this ticket. Didn't we agree in #33001 to keep the structure implementation and set some keywords argument triggers instead?

#33001 is about the structure argument to the manifold builder, which is not touched in this ticket here. To reference to #33001 is only insofar relevant as I realized during the discussion that the different structures (as defined in structure.py) are not really needed; and this ticket here implements this idea.

---

Why do you change this?

             # trivial cases
             if self == 1:
-                self._mixed_forms[nab] = A(dom._one_scalar_field)
+                self._mixed_forms[nab] = A(dom.scalar_field_algebra().one())
             elif self == 0:
-                self._mixed_forms[nab] = A(dom._zero_scalar_field)
+                self._mixed_forms[nab] = A(dom.scalar_field_algebra().zero())

Previously, the fields _scalar_field_algebra as well as _one_scalar_field and _zero_scalar_field were set in the constructor of TopologicalManifold. I removed these variables because now the differentiable manifold creates the correct scalar field algebra by overwriting scalar_field_algebra. I think this strategy also fixed a bug where an open subset of a diff manifold only had the scalar algebra of a topological manifold, see one of the changed doctests. (Also the code previously referenced protected variables from outside the class which is usually not a good idea.)

Another thing I noticed already in other tickets:

         for dom in domain.open_supersets():
-            dom._zero_scalar_field._express[self] = self.function_ring().zero()
-            dom._one_scalar_field._express[self] = self.function_ring().one()
+            dom.scalar_field_algebra().zero()._express[
+                self
+            ] = self.function_ring().zero()
+            dom.scalar_field_algebra().one()._express[self] = self.function_ring().one()

Imho, line breaks like this make the code much harder to read.

That's because of the line length limit (of 80?). I usually use black to format the code I've changed so that it adheres to this limit (and to fix other pep8 issues automatically).

In particular, I don't like that you add typing for just some parts. I'd prefer if we add typing for everything at once or leave things as they are (for now), but not some kind of hybrid.

I think it's just not feasible to fully type all of sage (or even of a single class) in one ticket. I think the spirit "add typing for the code you touch" seems a nice guide, because then the developer and reviewer are already familiar with the corresponding code. But I'm happy to follow a different approach if that's the consensus. (Also note Frédéric Chapoton already added a bit of typing here and there in other tickets, just not for manifolds.)

[sagetrac-git]

Changed commit from 9bc4cb1 to e46ada8