Metaticket - SageManifolds Code Improvement Discussion

[mjungmath]

The main purpose of this ticket is a discussion and coordination platform regarding general code improvements of the preexisting SageManifolds code apart from bugfixes and new features. That is, for instance, the removal of existing code duplication, and strategies to improve readability, debugging and compatibility.

I for myself do not have much experience in this field. However, I am eager to improve this nice piece of package as much as I can, so that anyone can benefit from it. In any case, this is not a one man task and I appreciate any support in this matter.

I am looking forward to the discussion! :)

Tasks:

• Inheritance Structure: Tensor Fields -> Vec. Bdl. Sections #29234: Remove code redundancies between Section and TensorField

CC: @egourgoulhon @tscrim @tobiasdiez @fchapoton @mkoeppe @nthiery

Component: manifolds

Keywords: sagemanifolds

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

[mjungmath]

comment:1

Allow me to start the discussion. In my opinion, the highest priority has the removal of code duplication. The new feature of vector bundles, for example, came with a cost: code duplication and redundancies. This is certainly unwanted (see ticket #29234). Take sections and tensor fields. They share a lot of common code snippets and methods. For example the method restrict which is more or less the same. Bugs for sections must now be fixed for tensor fields, and vice versa. But a simple inheritation comes with a problem: we have to drop most of the preexisting documentation. Imho, this would be sad, and I like to discuss how we deal with that.

Of course, this issue is not only restricted to vector bundles. As I modified tensor fields, I had to make changes in three different files simultaneously which had a very similar setup with similar code snippets: manifolds/differentiable/tensorfield.py, manifolds/differentiable/tensorfield_paral.py and tensor/modules/free_module_tensor.py. I had to copy the very same lines over and over again. It was a tedious task not only to modify the code but also to understand how it was built up. A simplification would be desireable, even though I have no clue how. If I look through the code, I am sure, I will find further examples.

Another thing I am bothering with is the invocation of private variables via ._private_variable. I think, these variables are private for a reason, and in my opinion, to keep the code as clean as possible, they should be invoced by methods and not directly. But I think, this has low priority because it has not such a tremendous impact. However, it would be nice, I guess.

[mjungmath]

Description changed:

--- 
+++ 
@@ -1,4 +1,4 @@
-The main purpose of this ticket is a discussion and coordination platform regarding general code improvements of the preexisting SageManifolds code, which do not involve bugfixes or new features. That is, for instance, the removal of existing code duplication, and strategies to improve readibility, debugging and compatibility.
+The main purpose of this ticket is a discussion and coordination platform regarding general code improvements of the preexisting SageManifolds code apart from bugfixes and new features. That is, for instance, the removal of existing code duplication, and strategies to improve readibility, debugging and compatibility.
 
 I for myself do not have much experience in this field. However, I am eager to improve this nice piece of package as much as I can, so that anyone can benefit from it. In any case, this is not a one man task and I appreciate any support in this matter.
 

[mkoeppe]

comment:4

Let me preface my comments: I have only very recently started to look into sage-manifolds and my understanding of this code is very shallow.

Replying to @mjungmath:

The new feature of vector bundles, for example, came with a cost: code duplication and redundancies. This is certainly unwanted (see ticket #29234). Take sections and tensor fields. They share a lot of common code snippets and methods. For example the method restrict which is more or less the same. Bugs for sections must now be fixed for tensor fields, and vice versa. But a simple inheritation comes with a problem: we have to drop most of the preexisting documentation. Imho, this would be sad, and I like to discuss how we deal with that.

If you are concerned about removing doctests (rather than documentation) as a byproduct of removing code duplications: There is a solution by moving many tests from doctests to _test... methods. These are inherited by subclasses and therefore test coverage can be improved.

I did this in the sage.numerical.backends modules in #20323 and follow-up tickets, and recently gh-kliem started to do the same in sage.geometry.polyhedra (see for example #29918).

In sage.numerical.backends.logging_backend I have some code that assists with transforming doctests to _test... methods, which could be generalized easily.

[mjungmath]

comment:5

Replying to @mkoeppe:

If you are concerned about removing doctests (rather than documentation) as a byproduct of removing code duplications: There is a solution by moving many tests from doctests to _test... methods. These are inherited by subclasses and therefore test coverage can be improved.

Thank you. This is good to know and definitely an important point. My concern, however, is also the documentation itself. Tensor fields and sections are declared slighly differently. For example, sections are constructed by using an instance of the vector bundle. Tensor fields, on the other hand, are declared by using an instance of the manifold. Therefore, it would be good to have separate closed minimal examples and hence distinct documentations.

What about something like this:

class MyClass(ParentClass)
    r"""
    Blabla
    """

    my_method(self, a, b, c='c'):
        r"""
        Overwritten docstring
        """
        return ParentClass.my_method(self, a, b, c=c)

Or is that too nasty?

[mkoeppe]

comment:6

Nothing wrong with that, I would say. (Of course, one would use super() instead of referring to the parent class by name.)
The performance impact should be minimal.

[nthiery]

comment:7

Just one caveat: it makes it more complicated to look at the code of the method by introspection.

I don't have a good solution though. The best approximation I can come it is to include both the examples/doc of vector bundles and tensor fields in the documentation of the method. It makes it a bit long for the reader. But points out the similarity, which is not bad per se.

Matthias: glad to see _test methods being adopted at a large scale, with additional tooling :-)

[mkoeppe]

comment:8

In a different direction: The (multi)linear algebra in sage.tensor that was developed for sage-manifolds has a much better design than the linear algebra provided by sage.matrix (see, for example, #30091).

It would be nice to be able to use it for computational (exact and inexact) (multi)linear algebra. In #30061, Eric already did some improvements that makes better use of sparsity.

In addition to such speedups, it would be good to be able to use different backends for storing tensors instead of the current dictionary backend.

On the one hand, for special cases such as vectors and matrices by existing special implementations; for the special case of fully symmetric tensors, using fast implementations of multivariate polynomials.

On the other hand, for general numerical tensors using mainstream accelerated frameworks like PyTorch (see #30096).

[mjungmath]

comment:9

Replying to @mkoeppe:

Nothing wrong with that, I would say. (Of course, one would use super() instead of referring to the parent class by name.)

I would have thought that referring to the parent class by name is more robust against changes and increases readability. Is there a particular reason to prefer super()?

The performance impact should be minimal.

That's good.

Replying to @nthiery:

Just one caveat: it makes it more complicated to look at the code of the method by introspection.

What exactly do you mean by that?

I don't have a good solution though. The best approximation I can come it is to include both the examples/doc of vector bundles and tensor fields in the documentation of the method. It makes it a bit long for the reader. But points out the similarity, which is not bad per se.

I'd guess this would contradict the well-documented philosophy of Sage, wouldn't it?

[mkoeppe]

comment:10

Replying to @mjungmath:

Replying to @mkoeppe:

(Of course, one would use super() instead of referring to the parent class by name.)

I would have thought that referring to the parent class by name is more robust against changes and increases readability. Is there a particular reason to prefer super()?

For classes with single inheritance, it is merely a matter of style. But as soon as multiple inheritance enters, using super() ensures that methods of all superclasses are called (and called in the right order)

[mkoeppe]

Description changed:

--- 
+++ 
@@ -1,4 +1,4 @@
-The main purpose of this ticket is a discussion and coordination platform regarding general code improvements of the preexisting SageManifolds code apart from bugfixes and new features. That is, for instance, the removal of existing code duplication, and strategies to improve readibility, debugging and compatibility.
+The main purpose of this ticket is a discussion and coordination platform regarding general code improvements of the preexisting SageManifolds code apart from bugfixes and new features. That is, for instance, the removal of existing code duplication, and strategies to improve readability, debugging and compatibility.
 
 I for myself do not have much experience in this field. However, I am eager to improve this nice piece of package as much as I can, so that anyone can benefit from it. In any case, this is not a one man task and I appreciate any support in this matter.