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

New implementation class FiniteRankDualFreeModule #30241

Closed
mkoeppe opened this issue Jul 28, 2020 · 30 comments
Closed

New implementation class FiniteRankDualFreeModule #30241

mkoeppe opened this issue Jul 28, 2020 · 30 comments

Comments

@mkoeppe
Copy link
Member

mkoeppe commented Jul 28, 2020

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

Depends on #34474

CC: @egourgoulhon @tscrim @mjungmath

Component: linear algebra

Author: Matthias Koeppe

Branch/Commit: 84d7b5e

Reviewer: Eric Gourgoulhon

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

@mkoeppe mkoeppe added this to the sage-9.2 milestone Jul 28, 2020
@mjungmath
Copy link

comment:1

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 28, 2020

comment:2

Replying to @mjungmath:

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 28, 2020

comment:3

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

@mjungmath
Copy link

comment:4

Replying to @mkoeppe:

Replying to @mjungmath:

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 28, 2020

comment:5

Replying to @mjungmath:

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

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 28, 2020

comment:6

Replying to @mjungmath:

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?

@mjungmath
Copy link

comment:7

Replying to @mkoeppe:

Replying to @mjungmath:

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 28, 2020

comment:8

Replying to @mjungmath:

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

@mjungmath
Copy link

comment:9

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

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 28, 2020

comment:10

Replying to @mjungmath:

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

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 28, 2020

comment:11

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

@mjungmath
Copy link

comment:12

Replying to @mkoeppe:

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.

@mkoeppe mkoeppe modified the milestones: sage-9.2, sage-9.3 Aug 13, 2020
@mkoeppe
Copy link
Member Author

mkoeppe commented Feb 13, 2021

comment:14

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

@mkoeppe mkoeppe modified the milestones: sage-9.3, sage-9.4 Feb 13, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.4, sage-9.5 Jul 19, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.5, sage-9.6 Dec 14, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.6, sage-9.7 Mar 5, 2022
@mkoeppe mkoeppe modified the milestones: sage-9.7, sage-9.8 Aug 31, 2022
@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 1, 2022

Dependencies: #34474

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 1, 2022

comment:20

Ticket description needs updating after #34474

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 2, 2022

Author: Matthias Koeppe

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 2, 2022

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 2, 2022

Commit: 37e41b9

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 2, 2022

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

37e41b9FiniteRankDualFreeModule.tensor_type: New; add tensor_product doctests

@egourgoulhon
Copy link
Member

comment:26

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?

@egourgoulhon
Copy link
Member

comment:27

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 4, 2022

Changed commit from 37e41b9 to 84d7b5e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 4, 2022

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

5044024FiniteRankDualFreeModule: Doc fixes
84d7b5eExtPower[Dual]FreeModule, FiniteRankDualFreeModule: Add docstring to construction method

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 4, 2022

comment:29

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

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 4, 2022

comment:30

Replying to Eric Gourgoulhon:

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.

@egourgoulhon
Copy link
Member

comment:31

Replying to Matthias Köppe:

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?

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 6, 2022

Reviewer: Eric Gourgoulhon

@mjungmath
Copy link

comment:33

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?

@vbraun
Copy link
Member

vbraun commented Sep 22, 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

4 participants