Opened 2 years ago
Closed 3 months ago
#30241 closed enhancement (fixed)
New implementation class FiniteRankDualFreeModule
Reported by:  Matthias Köppe  Owned by:  

Priority:  major  Milestone:  sage9.8 
Component:  linear algebra  Keywords:  
Cc:  Eric Gourgoulhon, Travis Scrimshaw, Michael Jung  Merged in:  
Authors:  Matthias Koeppe  Reviewers:  Eric Gourgoulhon 
Report Upstream:  N/A  Work issues:  
Branch:  84d7b5e (Commits, GitHub, GitLab)  Commit:  84d7b5eabff42af064f875ab7a0dca0354892d3a 
Dependencies:  #34474  Stopgaps: 
Description (last modified by )
(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).
Change History (34)
comment:2 followup: 4 Changed 2 years ago by
Replying to ghmjungmath:
This doesn't sound sensible to me.
ExtPowerDualFreeModule
andTensorFreeModule
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
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 followups: 5 6 10 Changed 2 years ago by
Replying to mkoeppe:
Replying to ghmjungmath:
This doesn't sound sensible to me.
ExtPowerDualFreeModule
andTensorFreeModule
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)
andM.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.
comment:5 Changed 2 years ago by
Replying to ghmjungmath:
... 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 followup: 7 Changed 2 years ago by
Replying to ghmjungmath:
M.exterior_power(1)
should return an instance ofExtPowerFreeModule
andM.tensor_module(1, 0)
should return an instance ofTensorFreeModule
.
Let's see. In your opinion, what should M.exterior_power(0)
and M.dual_exterior_power(0)
return?
comment:7 followup: 8 Changed 2 years ago by
Replying to mkoeppe:
Replying to ghmjungmath:
M.exterior_power(1)
should return an instance ofExtPowerFreeModule
andM.tensor_module(1, 0)
should return an instance ofTensorFreeModule
.Let's see. In your opinion, what should
M.exterior_power(0)
andM.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 Changed 2 years ago by
Replying to ghmjungmath:
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:10 Changed 2 years ago by
Replying to ghmjungmath:
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 followup: 12 Changed 2 years ago by
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 Changed 2 years ago by
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.
comment:13 Changed 2 years ago by
Milestone:  sage9.2 → sage9.3 

comment:14 Changed 22 months ago by
Milestone:  sage9.3 → sage9.4 

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.
comment:15 Changed 17 months ago by
Milestone:  sage9.4 → sage9.5 

comment:16 Changed 12 months ago by
Milestone:  sage9.5 → sage9.6 

comment:17 Changed 9 months ago by
Milestone:  sage9.6 → sage9.7 

comment:18 Changed 3 months ago by
Milestone:  sage9.7 → sage9.8 

comment:19 Changed 3 months ago by
Dependencies:  → #34474 

comment:21 Changed 3 months ago by
Description:  modified (diff) 

comment:22 Changed 3 months ago by
Authors:  → Matthias Koeppe 

comment:23 Changed 3 months ago by
Branch:  → u/mkoeppe/new_implementation_class_finiterankdualfreemodule 

comment:24 Changed 3 months ago by
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
Status:  new → needs_review 

comment:26 followup: 30 Changed 3 months ago by
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
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
Commit:  37e41b9b5f3968e32967128bd94df4f101b685bb → 84d7b5eabff42af064f875ab7a0dca0354892d3a 

comment:29 followup: 31 Changed 3 months ago by
I left the docstring of TensorFreeModule.construction
as is because it's getting replaced in #34448 already
comment:30 Changed 3 months ago by
Replying to Eric Gourgoulhon:
It would also be nice to update the
AUTHORS
field offinite_rank_free_module.py
.
I'll do this in a followup ticket to avoid a merge conflict with #30229.
comment:31 Changed 3 months ago by
Status:  needs_review → positive_review 

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?
comment:32 Changed 3 months ago by
Reviewers:  → Eric Gourgoulhon 

comment:33 Changed 3 months ago by
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
Branch:  u/mkoeppe/new_implementation_class_finiterankdualfreemodule → 84d7b5eabff42af064f875ab7a0dca0354892d3a 

Resolution:  → fixed 
Status:  positive_review → closed 
This doesn't sound sensible to me.
ExtPowerDualFreeModule
andTensorFreeModule
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.