Opened 2 years ago

Closed 3 months ago

# New implementation class FiniteRankDualFreeModule

Reported by: Owned by: Matthias Köppe major sage-9.8 linear algebra Eric Gourgoulhon, Travis Scrimshaw, Michael Jung Matthias Koeppe Eric Gourgoulhon N/A 84d7b5e 84d7b5eabff42af064f875ab7a0dca0354892d3a #34474

(from #30169)

We have the following identifications:

```sage: M = FiniteRankFreeModule(ZZ, 3, name='M')
sage: M is M.exterior_power(1)
True
sage: M is M.tensor_module(1, 0)
True
```

In contrast (in 9.7.rc0):

```sage: M.dual() is M.dual_exterior_power(1)
True
sage: M.dual() is M.tensor_module(0, 1)
False
```

After #34474:

```sage: M.dual() is M.dual_exterior_power(1)
True
sage: M.dual() is M.tensor_module(0, 1)
True
```

After #34474, the dual is implemented as a special case of `ExtPowerDualFreeModule`.

We create a separate implementation class `FiniteRankDualFreeModule` for it instead. We equip it with a `tensor_type` method, which allows the dual to participate in tensor products (see #34471).

### comment:1 follow-up:  2 Changed 2 years ago by Michael Jung

This doesn't sound sensible to me. `ExtPowerDualFreeModule` and `TensorFreeModule` follow very different construction patterns. For a reason: even from the mathematical point of view, one forms and (0,1) tensors are constructed differently, though they are isomorphic.

From that perspective, the current implementation makes perfect sense. What would make more sense is implementing the isomorphism.

Last edited 2 years ago by Michael Jung (previous) (diff)

### comment:2 in reply to:  1 ; follow-up:  4 Changed 2 years ago by Matthias Köppe

This doesn't sound sensible to me. `ExtPowerDualFreeModule` and `TensorFreeModule` follow very different construction patterns. For a reason: mathematically, meaning very strictly speaking, one forms and (0,1) tensors are defined differently, though they are isomorphic.

The same is true for `M.exterior_power(1)` and `M.tensor_module(1, 0)`, but we do have this identification.

### comment:3 Changed 2 years ago by Matthias Köppe

By the way, I am adding the additional structure of the exterior powers as quotients of tensor modules in #30169. Please take a look

### comment:4 in reply to:  2 ; follow-ups:  5  6  10 Changed 2 years ago by Michael Jung

This doesn't sound sensible to me. `ExtPowerDualFreeModule` and `TensorFreeModule` follow very different construction patterns. For a reason: mathematically, meaning very strictly speaking, one forms and (0,1) tensors are defined differently, though they are isomorphic.

The same is true for `M.exterior_power(1)` and `M.tensor_module(1, 0)`, but we do have this identification.

Fair point. Then I would vote for changing this to the expected outputs rather than creating a whole new class which has no further purpose than everything that is already there. Regarding Travis comment:10 this would be a convenient solution. Meaning: `M.exterior_power(1)` should return an instance of `ExtPowerFreeModule` and `M.tensor_module(1, 0)` should return an instance of `TensorFreeModule`. Then, one can implement the isomorphisms.

Either way, I agree that consistency is desirable here.

Allow me a side note: please remember that the whole manifold setup is built upon `FiniteRankFreeModule`. Modifying substatial things here might cause problems in the manifold implementation. It would be good to double check and, if absolutely necessary, make changes there, too. Henceforth, I suggest doctesting the manifold part, too.

Last edited 2 years ago by Michael Jung (previous) (diff)

### comment:5 in reply to:  4 Changed 2 years ago by Matthias Köppe

... a whole new class which has no further purpose than everything that is already there.

Note that by creating the new class, both of the `ExtPowerDualFreeModule` and `TensorFreeModule` classes will be simplified because they no longer have to implement the special case.

### comment:6 in reply to:  4 ; follow-up:  7 Changed 2 years ago by Matthias Köppe

`M.exterior_power(1)` should return an instance of `ExtPowerFreeModule` and `M.tensor_module(1, 0)` should return an instance of `TensorFreeModule`.

Let's see. In your opinion, what should `M.exterior_power(0)` and `M.dual_exterior_power(0)` return?

### comment:7 in reply to:  6 ; follow-up:  8 Changed 2 years ago by Michael Jung

`M.exterior_power(1)` should return an instance of `ExtPowerFreeModule` and `M.tensor_module(1, 0)` should return an instance of `TensorFreeModule`.

Let's see. In your opinion, what should `M.exterior_power(0)` and `M.dual_exterior_power(0)` return?

Strictly speaking, they should return instances of `ExtPowerFreeModule` or `ExtPowerDualFreeModule` respectively, which are isomorphic to the base ring.

You definitely have a good point. And I totally agree that this should be unified, in either way.

But I am strictly against a new class dedicated to just one special case. At least, this is how I understand your proposal.

### comment:8 in reply to:  7 Changed 2 years ago by Matthias Köppe

I am strictly against a new class dedicated to just one special case.

I'll just prepare a branch and then you can see how it simplifies things, which will make it easier to review this proposal

### comment:9 Changed 2 years ago by Michael Jung

Sure, I will take a look. Travis, what do you say?

### comment:10 in reply to:  4 Changed 2 years ago by Matthias Köppe

Allow me a side note: please remember that the whole manifold setup is built upon `FiniteRankFreeModule`. Modifying substatial things here might cause problems in the manifold implementation. It would be good to double check and, if absolutely necessary, make changes there, too. Henceforth, I suggest doctesting the manifold part, too.

Thanks. Prompted by your remark, I have taken a look and I noticed that `VectorFieldModule` also has similar code. I have yet to study this code in detail (I am slowly making my way up the stack...), but it looks it has the same inconsistency between primal and dual:

```sage: M = Manifold(2, 'M')
sage: XM = M.vector_field_module()
sage: XM.tensor_module(1, 0) is XM
True
sage: XM.tensor_module(0, 1) is XM.dual()
False
```

### comment:11 follow-up:  12 Changed 2 years ago by Matthias Köppe

I think I have a solution that could make everyone happy.

In #30169, I had implemented `FiniteRankDualFreeModule.__classcall_private__` methods that delegate to other classes and implement the identification.

But we could as well take care of all identifications in the `FiniteRankModule.exterior_power`, `dual_power`, `tensor_module` methods.

Then the class constructors `ExtPowerFreeModule` could keep the strict behavior (I would also check that the case of degree 0 is correctly implemented - I don't think it is currently).

### comment:12 in reply to:  11 Changed 2 years ago by Michael Jung

I think I have a solution that could make everyone happy.

In #30169, I had implemented `FiniteRankDualFreeModule.__classcall_private__` methods that delegate to other classes and implement the identification.

But we could as well take care of all identifications in the `FiniteRankModule.exterior_power`, `dual_power`, `tensor_module` methods.

This sounds much better, yes. It would also be good to leave a note in the documentation so that the user knows about these special cases and how they are treated (if not already done).

Then the class constructors `ExtPowerFreeModule` could keep the strict behavior (I would also check that the case of degree 0 is correctly implemented - I don't think it is currently).

I would appreciate this task. The degree zero case had already caused some troubles in the past.

### comment:13 Changed 2 years ago by Matthias Köppe

Milestone: sage-9.2 → sage-9.3

### comment:14 Changed 22 months ago by Matthias Köppe

Milestone: sage-9.3 → sage-9.4

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

### comment:15 Changed 17 months ago by Matthias Köppe

Milestone: sage-9.4 → sage-9.5

### comment:16 Changed 12 months ago by Matthias Köppe

Milestone: sage-9.5 → sage-9.6

### comment:17 Changed 9 months ago by Matthias Köppe

Milestone: sage-9.6 → sage-9.7

### comment:18 Changed 3 months ago by Matthias Köppe

Milestone: sage-9.7 → sage-9.8

### comment:19 Changed 3 months ago by Matthias Köppe

Dependencies: → #34474

### comment:20 Changed 3 months ago by Matthias Köppe

Ticket description needs updating after #34474

### comment:21 Changed 3 months ago by Matthias Köppe

Description: modified (diff)

### comment:22 Changed 3 months ago by Matthias Köppe

Authors: → Matthias Koeppe

### comment:23 Changed 3 months ago by Matthias Köppe

Branch: → u/mkoeppe/new_implementation_class_finiterankdualfreemodule

### comment:24 Changed 3 months ago by git

Commit: → 37e41b9b5f3968e32967128bd94df4f101b685bb

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

 ​37e41b9 `FiniteRankDualFreeModule.tensor_type: New; add tensor_product doctests`

### comment:25 Changed 3 months ago by Matthias Köppe

Status: new → needs_review

### comment:26 follow-up:  30 Changed 3 months ago by Eric Gourgoulhon

Looks good, thanks. There are some minor issues regarding the documentation of class `FiniteRankDualFreeModule`:

```-     the *dual of* `M` is the set `M^*` of all linear forms `p` on `M`,
+     the *dual of* `M` is the set `M^*` of all linear forms on `M`,
```
```-    - ``name`` -- (default: ``None``) string; name given to `\Lambda^p(M^*)`
+    - ``name`` -- (default: ``None``) string; name given to `M^*`
```
```-    EXAMPLES:
+    EXAMPLES::
```

It would also be nice to update the `AUTHORS` field of `finite_rank_free_module.py`.

You probably already know it, but just in case: instead of running `make doc` (which is so slow that one always hesitate to run it), a fast way to generate the documentation regarding tensors on finite rank free modules is

```sage -docbuild reference/tensor_free_modules html
```

The documentation is then located in `local/share/doc/sage/html/en/reference/tensor_free_modules/index.html` under the Sage root directory.

Michael, do you have any further comments?

### comment:27 Changed 3 months ago by Eric Gourgoulhon

Another issue regardng the documentation: in the html doc, the method `construction()` of `FiniteRankDualFreeModule` appears as an empty item, which is somewhat weird. This is because its docstring contains only a `TESTS` field. There should be at least something like "Not implemented yet" and/or the comment that appears only in the Python code (`No construction until we extend VectorFunctor with a parameter 'dual'`). Same remark about the `construction()` methods of `TensorFreeModule`, `ExtPowerFreeModule` and `ExtPowerDualFreeModule` after #30235.

### comment:28 Changed 3 months ago by git

Commit: 37e41b9b5f3968e32967128bd94df4f101b685bb → 84d7b5eabff42af064f875ab7a0dca0354892d3a

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

 ​5044024 `FiniteRankDualFreeModule: Doc fixes` ​84d7b5e `ExtPower[Dual]FreeModule, FiniteRankDualFreeModule: Add docstring to construction method`

### comment:29 follow-up:  31 Changed 3 months ago by Matthias Köppe

I left the docstring of `TensorFreeModule.construction` as is because it's getting replaced in #34448 already

### comment:30 in reply to:  26 Changed 3 months ago by Matthias Köppe

It would also be nice to update the `AUTHORS` field of `finite_rank_free_module.py`.

I'll do this in a follow-up ticket to avoid a merge conflict with #30229.

### comment:31 in reply to:  29 Changed 3 months ago by Eric Gourgoulhon

Status: needs_review → positive_review

I left the docstring of `TensorFreeModule.construction` as is because it's getting replaced in #34448 already

OK. Thanks for the other changes.

Michael, do you agree with the positive review?

### comment:32 Changed 3 months ago by Matthias Köppe

Reviewers: → Eric Gourgoulhon

### comment:33 Changed 3 months ago by Michael Jung

Sorry, it's a long time ago I looked into this.

We want tensor products for dual elements, am I getting this right? Is the long term goal then to implement `ExtPowerDualFreeModule` as a quotient of tensor powers of `FiniteRankDualFreeModule`?

### comment:34 Changed 3 months ago by Volker Braun

Branch: u/mkoeppe/new_implementation_class_finiterankdualfreemodule → 84d7b5eabff42af064f875ab7a0dca0354892d3a → fixed positive_review → closed
Note: See TracTickets for help on using tickets.