Opened 18 months ago
Closed 6 months ago
#15916 closed enhancement (fixed)
Tensors on free modules of finite rank
Reported by: | egourgoulhon | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.5 |
Component: | linear algebra | Keywords: | free module, tensor, tensor product, days64 |
Cc: | SimonKing | Merged in: | |
Authors: | Eric Gourgoulhon, Michal Bejger | Reviewers: | Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | 99f9ac5 (Commits) | Commit: | 99f9ac53bd3e42dd00f5d05b7b126a632161ca55 |
Dependencies: | Stopgaps: |
Description (last modified by egourgoulhon)
Description
This ticket implements:
- tensor products of the type M\otimes ...\otimes M \otimes M* \otimes...\otimes M* where M is a free module of finite rank over a commutative ring R and M* is its dual (k factors of M and l factors of M*, say)
- the elements of the above tensor products, considered as tensors of type (k,l) on M, i.e. multilinear forms (M*)^{k} \times M^{l} --> R, thanks to the canonical isomorphism (M*)* = M (which holds since M is a free module of finite rank)
- the following tensor operations:
- operations inherent to the module structure (addition, multiplication by a ring element)
- tensor product of two tensors
- tensor contraction
- symmetry / antisymmetry handling (on subset of the tensor arguments or on all arguments)
- exterior product of alternating forms
- morphisms of free modules of finite rank, with coercion of endomorphisms to/from type-(1,1) tensors
No distinguished basis is assumed on the free module M; on the contrary many bases can be introduced. Each tensor has then various representations, via its components in the various bases.
Motivation and context
The ticket has been motivated by tensors on smooth manifolds over R, within the
SageManifolds project. In this context, tensors on free modules appear at two levels:
- tensors on tangent spaces:
- commutative ring R: real field R
- free module M: tangent vector space at a given manifold's point
- tensor fields on a manifold:
- commutative ring R: the algebra C^{oo}(U) of smooth functions U--> R, where U is a parallelizable open set of the manifold
- free module M: the set X(U) of smooth vector fields on U (since U is parallelizable, this is a free module; its rank is the manifold's dimension)
Documentation
Apart from the numerous doctests in the code, some pieces of documentation are
- the tutorial worksheet posted here (a pdf version is here)
- the "tensors on free modules" reference manual; it can also be generated via the command sage -docbuild reference/tensor_free_modules html
See also this page.
Remarks
- Although developed in the context of SageManifolds (ticket:14865), the ticket is self-contained and does not depend on other parts of SageManifolds. It this respect, it can be viewed as some attempt to include a first subset of SageManifolds in Sage, with a moderate size: the ticket comprises 14499 lines of Python code (most of them being doctests), while at present (version 0.6) SageManifolds contains 35157 lines of code.
- The ticket follows Sage's Parent/Element pattern and the (new) category framework. In particular, the ticket's free module class (FiniteRankFreeModule) passes the module TestSuite.
- It turned out to be necessary to develop a new class to implement free modules of finite rank. Indeed, the category of free modules does not exist yet in Sage: only those of generic modules (Modules) or free modules with a distinguished basis (ModulesWithBasis) are available. Now, the tangent space at a given point of a manifold is a vector space without any distinguished basis (in other words, while the tangent space is isomorphic to R^{n}, there is no canonical isomorphism, each isomorphism relying on the choice of some coordinate chart). The new class, FiniteRankFreeModule, does not rely on any distinguished basis. It inherits directly from Parent, with the category set to Modules(). In particular, it does not inherit from sage.modules.module.FreeModule_generic since the latter seems to assume a distinguished basis (cf. its method basis()).
Change History (66)
comment:1 Changed 18 months ago by egourgoulhon
comment:2 Changed 18 months ago by egourgoulhon
- Description modified (diff)
comment:3 Changed 17 months ago by git
- Commit changed from f8e9c3e62c1b016a41d377a0ccdbcd665aa189dc to e34f29ec69a4595f52894724a5aec22a571c02ce
comment:4 Changed 17 months ago by jhpalmieri
Is there any connection and/or overlap between this ticket and #15726?
comment:5 Changed 17 months ago by egourgoulhon
This ticket implements tensor modules T^{(k,l)}(M) of a fixed type (k,l) over a free module M of finite rank, while #15726 implements the tensor algebra T(M), which is the direct sum of all T^{(k,l)}(M).
Another major difference is that here the base module M is a generic free module, while in #15726 M is a free module with a distinguished basis (the classes TensorModule and TensorAlgebra of #15726 inherit from class CombinatorialFreeModule, which assumes a distinguished basis). In the present ticket, an arbitrary number of bases can be introduced on the free module M, along with the change-of-basis automorphisms, and each tensor has various sets of components, one per basis.
Another difference may regard symmetry/antisymmetry handling: the tensors implemented in this ticket can have arbitrary symmetries and/or antisymmetries. The symmetries are taken into account to reduce the storage of tensor components and to optimize some computations, such as the tensor product. But maybe something similar is implemented in #15726.
comment:6 Changed 16 months ago by vbraun_spam
- Milestone changed from sage-6.2 to sage-6.3
comment:7 Changed 16 months ago by git
- Commit changed from e34f29ec69a4595f52894724a5aec22a571c02ce to 8b7bd605a0b41f3e428c6e67bb85d630d193065a
Branch pushed to git repo; I updated commit sha1. New commits:
8b7bd60 | Updated 'tensors on free modules' to Sage 6.2 |
comment:8 Changed 16 months ago by git
- Commit changed from 8b7bd605a0b41f3e428c6e67bb85d630d193065a to 010c31309df79e4957bb7e8daca6cd47d22b2121
Branch pushed to git repo; I updated commit sha1. New commits:
010c313 | Minor improvements; coincides with SageManifolds as of 18 May 2014 (commit d488f8f807 on github) |
comment:9 Changed 14 months ago by git
- Commit changed from 010c31309df79e4957bb7e8daca6cd47d22b2121 to 267b6b937226357d87669f7c5c8c699d99188b5a
Branch pushed to git repo; I updated commit sha1. New commits:
267b6b9 | Update to SageManifolds v0.5 |
comment:10 Changed 14 months ago by egourgoulhon
This is now part of SageManifolds v0.5 (see this post on sage-devel and http://sagemanifolds.obspm.fr/changelog.html).
comment:11 Changed 14 months ago by egourgoulhon
- Status changed from new to needs_review
comment:12 Changed 14 months ago by egourgoulhon
- Description modified (diff)
comment:13 Changed 14 months ago by egourgoulhon
- Description modified (diff)
comment:14 Changed 14 months ago by egourgoulhon
- Description modified (diff)
comment:15 Changed 14 months ago by SimonKing
- Cc SimonKing added
comment:16 Changed 13 months ago by vbraun_spam
- Milestone changed from sage-6.3 to sage-6.4
comment:17 Changed 13 months ago by git
- Commit changed from 267b6b937226357d87669f7c5c8c699d99188b5a to 8f0878d7c04e15559a76c427fac9a764fa1ecca8
Branch pushed to git repo; I updated commit sha1. New commits:
8f0878d | Added index notation for tensor contractions and symmetrizations |
comment:18 Changed 11 months ago by git
- Commit changed from 8f0878d7c04e15559a76c427fac9a764fa1ecca8 to d8f518ff48c8be2ea73f2b51e067af9e6c87b4bf
Branch pushed to git repo; I updated commit sha1. New commits:
d8f518f | Update to SageManifolds v0.6 |
comment:19 Changed 11 months ago by egourgoulhon
The ticket has been updated to coincide with the pure algebraic part of SageManifolds v0.6. Changes are:
1/ The possibility to use index notations for denoting tensor contractions or symmetrizations: the indices have to be passed in LaTeX notations (possibly without {}) as a string inside the square bracket operator, e.g.
sage: S = A['^{ab}_{cd}']*B['^d_a'] # equivalent to S = A.contract(0, 3, B, 1, 0)
to denote the tensor S^{b}_{c} = A^{ab}_{cd} B^{d}_{a} ,
sage: S = A['^{(ab)}_{cd}'] # equivalent to S = A.symmetrize(0,1)
to denote the tensor S^{ab}_{cd} = A^{(ab)}_{cd}, and
sage: S = A['^{ab}_{[cd]}'] # equivalent to S = A.antisymmetrize(2,3)
to denote the tensor S^{ab}_{cd} = A^{ab}_{[cd]}.
See this page and this one for more details.
2/ The code for tensor contractions has been completely rewritten; it is more efficient and allows now for multiple contractions (as in the first example above).
3/ The argument of methods symmetrize() and antisymmetrize() in the FreeModuleTensor class is now directly a sequence of index positions (and no longer a single list/tuple encapsulating such a sequence).
Besides, new examples have been provided on http://sagemanifolds.obspm.fr/examples.html; two of them are directly relevant to this ticket:
- tensors on free modules of finite rank (tutorial)
- vector-field modules on the 2-sphere (a concrete example of use)
comment:20 follow-up: ↓ 21 Changed 11 months ago by vbraun
Doctest coverage: sage -coverage src/sage/tensor/
I don't think the string notation is very user friendly, A['^{ab}_{cd}']*B['^d_a'] looks more like line noise. How about recognizing symbolic variables and the built-in function any as special:
sage: var('a, d')
sage: A[a, any, any, d] * B[d, a]
Or, perhaps better in the long run: Define your own index class that knows about whether indices are upper or lower. Then we can also check that you don't contract two upper indices, and/or automatically insert the metric there:
sage: a, b, c, d = A.indices()
sage: a
Upper index
sage: A[a, b, c, d] * B[d, a]
comment:21 in reply to: ↑ 20 Changed 11 months ago by egourgoulhon
Replying to vbraun:
Doctest coverage: sage -coverage src/sage/tensor/
OK I see: most private methods appear to have no doctest because their doctests have been put in the docstring of the class itself: this is the only way for them to appear in the generated html documentation. This is particularly true for the __init__ methods (there are actually a lot of doctests for them). Shall the doctests be repeated in the docstring of the private methods ? What is the general policy for this ?
I don't think the string notation is very user friendly, A['^{ab}_{cd}']*B['^d_a'] looks more like line noise. How about recognizing symbolic variables and the built-in function any as special:
sage: var('a, d')
sage: A[a, any, any, d] * B[d, a]
Your suggestion for any is quite compeling, thanks. Note that, in the present version, it is already possible to replace unrelevant indices with dots, which decreases the noise:
sage: S = A['^{a.}_{.d}']*B['^d_a']
Or, perhaps better in the long run: Define your own index class that knows about whether indices are upper or lower. Then we can also check that you don't contract two upper indices, and/or automatically insert the metric there:
sage: a, b, c, d = A.indices()
sage: a
Upper index
sage: A[a, b, c, d] * B[d, a]
You are right: in the long run, we should probably introduce an indice class to get rid of the strings. The only advantage of the present notation is that we do not have to spoil some variable names to denote indices: they can be set on the fly, to denote an operation like contraction where index notation is more clear, most operations being performed in an index-free setting.
comment:22 Changed 11 months ago by vbraun
The __init__ docstring is used as the class docstring if you don't define a separate class docstring, and I would recommend using that.
You can document other underscore methods by adding, say,
.. automethod:: __mul__
at the end of the class docstring (possibly derived from the __init__ docstring as above).
Not all underscore methods need to appear in the reference manual, though. The doctests are still tests, even if they are not end-user visible "documentation". And they are documentation for developers reading the source and wondering how to use that method...
comment:23 follow-up: ↓ 24 Changed 11 months ago by tscrim
What I generally do is put all of the important info in the class docstring, like the INPUT block and examples showing use-cases. In the __init__ method, it is just creating a common example X and doing TestSuite(X).run() (and possibly repeating for some boarder cases).
Also what Volker is alluding to is that all methods, including underscore ones, must have some doctest.
Some quick comments/questions:
- Please remove the autogenerated files from git tracking (as they will be autogenerated :p).
- The imports from tensor/modules I feel like should be done using tensor/all.py rather than all.py (at the source root).
- I'm wondering if the tensor/modules doc should be built along with the tensor doc (rather than it's own separate thing) as tensor is small to begin with. However, I don't think there's any drawbacks to having it be separate since it's (essentially) disjoint from the other doc.
- Is there anything I can do with or you need from #15726? I'd like to make these compatible as much as possible.
- +1 to having a class for indices (once there is clear a need for it).
I really like the pictures on the Sage manifolds website and think this will be a great addition to Sage. Unfortunately I don't have time right now to do much of a review.
Best,
Travis
PS - You might also be interested in #9439.
comment:24 in reply to: ↑ 23 Changed 11 months ago by egourgoulhon
Replying to tscrim:
What I generally do is put all of the important info in the class docstring, like the INPUT block and examples showing use-cases. In the __init__ method, it is just creating a common example X and doing TestSuite(X).run() (and possibly repeating for some boarder cases).
Thanks to you and Volker for your advices.
Also what Volker is alluding to is that all methods, including underscore ones, must have some doctest.
OK got it. Will modify the code accordingly.
Some quick comments/questions:
- Please remove the autogenerated files from git tracking (as they will be autogenerated :p).
OK.
- The imports from tensor/modules I feel like should be done using tensor/all.py rather than all.py (at the source root).
Yes, this would be more consistent with the directory structure.
- I'm wondering if the tensor/modules doc should be built along with the tensor doc (rather than it's own separate thing) as tensor is small to begin with. However, I don't think there's any drawbacks to having it be separate since it's (essentially) disjoint from the other doc.
Actually the current content of tensor (algebra of differential forms defined on a coordinate patch of R^{n}) pertains more to smooth manifolds than to tensor modules over arbitrary commutative rings.
- Is there anything I can do with or you need from #15726? I'd like to make these compatible as much as possible.
Sure! There is clearly some overlap between the two and we should discuss this further. For instance, once a basis is chosen, a tensor from this ticket should be converted into an element of the tensor algebra of #15726. Let us keep in touch about this.
- +1 to having a class for indices (once there is clear a need for it).
I really like the pictures on the Sage manifolds website and think this will be a great addition to Sage. Unfortunately I don't have time right now to do much of a review.
Best,
Travis
PS - You might also be interested in #9439.
Yes this has clearly some connections with SageManifolds, as well as #10132 (now integrated into Sage) and your ticket about Lie algebras (#14901).
comment:25 Changed 11 months ago by git
- Commit changed from d8f518ff48c8be2ea73f2b51e067af9e6c87b4bf to 45aaa975d406ade603a15a34f34a9790936e42ab
Branch pushed to git repo; I updated commit sha1. New commits:
45aaa97 | Add doctests and improve documentation in tensor/modules |
comment:26 Changed 11 months ago by egourgoulhon
Hi,
- Following Volker's comment, doctests to all underscore methods have been added. Accordingly sage -coverage src/sage/tensor/modules results in 100% coverage.
- Following Travis' one, (i) I've suppressed the spurious mention "autogenerated file" in the rst files (this resulted from some cut-and-paste; actually these files are not autogenerated) and (ii) I've moved the import of tensor/modules to sage/tensor/all.py.
Some simple examples illustrating this ticket are on this page. Concrete examples of use in the framework of fields on a smooth manifold (modules over the algebra of scalar fields on S^{2}) are here (this requires #14865).
comment:27 Changed 11 months ago by egourgoulhon
- Description modified (diff)
comment:28 follow-up: ↓ 29 Changed 11 months ago by tscrim
You shouldn't need those .rst because there are such files autogenerated by Sage's docbuilder (although they are a little more buried in the /src/doc/en/reference/tensor/sage/, or whatever documentation group you're considering) and you're not doing anything different as far as I can see.
Also on a quick glance through the recent commit, I noticed a colon missing on in these two places:
@@ -984,6 +1179,21 @@ class Components(SageObject): - True if ``self`` is different from ``other``, or False otherwise + EXAMPLES: + + sage: from sage.tensor.modules.comp import Components @@ -176,12 +177,46 @@ class TensorFreeModule(FiniteRankFreeModule): [snip] Element = FreeModuleTensor def __init__(self, fmodule, tensor_type, name=None, latex_name=None): + r""" + TEST: + + sage: from sage.tensor.modules.tensor_free_module import TensorFreeModule
comment:29 in reply to: ↑ 28 Changed 11 months ago by egourgoulhon
Replying to tscrim:
Thanks for your comments.
You shouldn't need those .rst because there are such files autogenerated by Sage's docbuilder (although they are a little more buried in the /src/doc/en/reference/tensor/sage/, or whatever documentation group you're considering) and you're not doing anything different as far as I can see.
Actually, I've added these .rst files because they are not autogenerated in the present case. More precisely, the command sage -docbuild tensors_free_module html does not produce any sage subdirectory in src/doc/en/tensors_free_module. On the contrary the command sage -docbuild reference html produces a sage subdirectory in src/doc/en/reference/tensor_free_modules, which contains the autogenerated .rst files. As a result, if I simply copy the file index.rst from src/doc/en/reference/tensor_free_modules to src/doc/en/tensors_free_module and delete all the other .rst files (the "non-autogenerated" ones), I get the error message toctree contains reference to nonexisting document u'sage/tensor/modules/finite_rank_free_module'. Certainly, I am not doing something properly here. Any help on this would be appreciated...
/
Also on a quick glance through the recent commit, I noticed a colon missing on in these two places:
Thanks for pointing this; I'll correct it.
comment:30 follow-up: ↓ 31 Changed 11 months ago by vbraun
You should only have the modules/index.rst. The docbuilder will not always process incremental changes correctly, you might have to sage -b && make doc-clean && make doc
Edit: added the "not"
comment:31 in reply to: ↑ 30 Changed 11 months ago by egourgoulhon
Replying to vbraun:
You should only have the modules/index.rst. The docbuilder will not always process incremental changes correctly, you might have to sage -b && make doc-clean && make doc
I tried but it did not helped. To make things clear, let me stress that the problem arises because we wanted to have a separate reference manual for tensors on free modules, in a directory that we created for this purpose: src/doc/en/tensors_free_module, which differs from the corresponding section in the main Sage reference manual (directory src/doc/en/reference/tensor_free_modules). For the latter, everything works well: the only .rst file under src/doc/en/reference/tensor_free_modules is index.rst and it suffices for the docbuilder to produce the complete documentation. The problem arises only for src/doc/en/tensors_free_module. So the question might be: "how to produce a separate reference manual for some subpart of Sage library ?".
comment:32 follow-up: ↓ 33 Changed 11 months ago by vbraun
You can't and you shouldn't be able to. The top-level sections correspond to the src/sage subdirectories, as they should. You can, however, be quite fancy in src/doc/en/reference/modules/index.rst, see e.g. the combinat section.
comment:33 in reply to: ↑ 32 Changed 11 months ago by tscrim
Replying to vbraun:
You can't and you shouldn't be able to. The top-level sections correspond to the src/sage subdirectories, as they should. You can, however, be quite fancy in src/doc/en/reference/modules/index.rst, see e.g. the combinat section.
That's not true, there are significantly more top-level docs than top-level sections in src/sage, the first one that comes to mind is finite_fields. I'm taking a look at it right now to see what's going on.
comment:34 follow-up: ↓ 35 Changed 11 months ago by tscrim
- Branch changed from u/egourgoulhon/tensor_modules to public/tensor_modules-15916
- Commit changed from 45aaa975d406ade603a15a34f34a9790936e42ab to a07a21ae1170e224192f9fe6bae34825dae23e1d
I think the problem is you put things in src/doc/en/tensors_free_module, as opposed to src/doc/en/reference/tensors_free_module. So with this branch you should be able to run Volker's commands (or sage -docbuild all html or (what I did to check) sage -docbuild reference/tensor_free_modules inventory and sage -docbuild reference/tensor_free_modules html).
New commits:
93dcad2 | Merge branch 'u/egourgoulhon/tensor_modules' of trac.sagemath.org:sage into public/tensor_modules-15916 |
a07a21a | Fixing the documentation. |
comment:35 in reply to: ↑ 34 Changed 11 months ago by egourgoulhon
Replying to tscrim:
It works! Thank you very much Travis. This yields exactly what I had in mind. Moreover, it is nice that one can get rid of the directory src/doc/en/tensors_free_module and stay only with src/doc/en/reference/tensor_free_modules, for duplication is root of all evil ;-)
comment:36 Changed 11 months ago by git
- Commit changed from a07a21ae1170e224192f9fe6bae34825dae23e1d to c43321bf78adc4ac6ff324f910a879610742d383
Branch pushed to git repo; I updated commit sha1. New commits:
c43321b | Minor modifications in the documentation of tensor/modules. |
comment:37 Changed 9 months ago by git
- Commit changed from c43321bf78adc4ac6ff324f910a879610742d383 to 5fba412cef169dc7bbc1c32b7d254ab0bf185d4a
comment:38 follow-ups: ↓ 39 ↓ 41 Changed 9 months ago by tscrim
- Reviewers set to Travis Scrimshaw
- Status changed from needs_review to needs_work
Okay, I've made a first pass of changes. So overall it looks really good, but there's currently some issues that need addressing. Here's what I've changed:
- Removed trailing whitespace.
- Corrected some indentations.
- Changed lowercase repr output into Uppercase.
- Added an __eq__ method to TensorWithIndices.
- Fixed some doc formatting (there's a likely more to do on this). However you have 2 conventions, type-`(a,b)` tensor and type `(a,b)` tensor and IMO you should pick one and stick with it.
- Added TestSuite to all classes.
- Changed some TypeError's to ValueError's because the data type was correct but the values within were not.
- Made _an_element_ of the finite rank free module return an element with a basis.
- Made the __eq__ test of tensors check first by identity because it was really annoying to me that t == t raised an error.
- Removed the __hash__ and __eq__ of FiniteRankFreeModule since UniqueRepresentation takes care of it.
- Changed errors from This is an error message. to this is an error message as I believe it more standard.
- Some other misc. changes on some doctests and things like l != [] for not l (it's marginally faster).
Some questions:
- Do we need the class FormattedExpansion? It doesn't seem to be used.
- Why do you have _new_instance, couldn't you use __copy__?
- Should we make the morphisms inherit from Morphism and/or have their parents be some kind of Homset?
- Could we expand out some of the names, ex. comp() to components() (perhaps with aliases for the short names)?
- I'm not too happy about the name basis_change as I would try change_of_basis first, but within that method, could we also check transitivity (there's a method which does this already, but I forget what it's called)?
- In Sage, the view function outputs as a pdf. So the name of the view() method is confusing to me. Do you think we could change the name to something else like expand?
Some things to do:
- Implement a _matrix_ method so (1,1)-tensors t can be passed as matrix(t).
- Fix all failures from the TestSuite or justify why we can skip certain tests.
There will be more to do I think, but it probably won't be too major.
comment:39 in reply to: ↑ 38 Changed 9 months ago by egourgoulhon
Replying to tscrim:
Okay, I've made a first pass of changes. So overall it looks really good, but there's currently some issues that need addressing. Here's what I've changed:
Thank you very much for your work on this ticket! I am looking at the diff and I am impressed by the amount of things that you have changed! This is very instructive to me.
Some questions:
I shall reply later to your questions, but I can already say that I implemented morphisms last week, in the working repository of SageManifolds at https://github.com/sagemanifolds/sage. The free module morphisms inherit from Morphism and their parent class from Homset. Coercions have been implemented between endomorphisms and type-(1,1) tensors. I shall merge this branch into the ticket branch this week.
Some things to do:
- Implement a _matrix_ method so (1,1)-tensors t can be passed as matrix(t).
- Fix all failures from the TestSuite or justify why we can skip certain tests.
OK I will work on this in the coming weeks.
comment:40 Changed 9 months ago by git
- Commit changed from 5fba412cef169dc7bbc1c32b7d254ab0bf185d4a to 6e53a8d3c92d405541e2a21dff5a867d402937e6
Branch pushed to git repo; I updated commit sha1. New commits:
6e53a8d | Add free module homomorphisms, rename some classes and methods and make small improvements. |
comment:41 in reply to: ↑ 38 ; follow-up: ↓ 44 Changed 9 months ago by egourgoulhon
Replying to tscrim:
Some questions:
- Do we need the class FormattedExpansion? It doesn't seem to be used.
Yes, we need it: it is used by the method view() of classes FreeModuleTensor and FreeModuleAltForm, to have a nice output, either text formatted or LaTeX formatted (notebook).
- Why do you have _new_instance, couldn't you use __copy__?
Actually _new_instance is not performing a copy: it simply creates an instance of the same type and same general characteristics (e.g. tensor type), but leave it empty (i.e. it does not perform any copy of stored data, like components).
- Should we make the morphisms inherit from Morphism and/or have their parents be some kind of Homset?
This is done in the new commit. It introduces free module homomorphisms, via
- the parent class FreeModuleHomset (subclass of sage.categories.homset.Homset)
- the element class FiniteRankFreeModuleMorphism (subclass of sage.categories.morphism.Morphism)
Coercions to/from type-(1,1) tensors have been introduced as well.
- Could we expand out some of the names, ex. comp() to components() (perhaps with aliases for the short names)?
This is done in the new commit, with comp() as an alias to components() (since it is really usefull to have a short name to access to components).
- I'm not too happy about the name basis_change as I would try change_of_basis first,
Yes, you are right: change_of_basis is a better name. The change has been performed in the new commit.
but within that method, could we also check transitivity (there's a method which does this already, but I forget what it's called)?
Not done yet. Note however that transitivity is implemented in the method FreeModuleTensor.common_basis().
- In Sage, the view function outputs as a pdf. So the name of the view() method is confusing to me. Do you think we could change the name to something else like expand?
We thought quite a lot about the name of this method; here we really need a short name, because it is used very often in interactive mode (notebook). The idea is to display tensors in a nice human readable form, but not to change their state, which something like expand might suggest. display was too long. An alternative was show but we eliminated it because in Sage it is used for graphic outputs. So the short name that we ended with was view. We may also wonder why view should be reserved for pdf output. Wouldn't view_pdf or make_pdf be more suggestive ?
Some things to do:
- Implement a _matrix_ method so (1,1)-tensors t can be passed as matrix(t).
This is done in the new commit, but on components instead of tensors (indeed, constructing a matrix from a tensor would have required an extra argument: the basis, to define which components are used). So currently, if t is a type-(1,1) tensor and e is a module basis, we have matrix(t.components(e)) yielding a matrix of base ring elements representing the components of t w.r.t. basis e.
- Fix all failures from the TestSuite or justify why we can skip certain tests.
Done. Some fixes consisted in initializing the objects (e.g. by setting some components) prior to the TestSuite. Note that _test_category had to be skipped when constructing the object by a direct call to __init__, since the correct construction must be performed by a call to parent().element_class. In the latter case, _test_category passed.
comment:42 Changed 9 months ago by egourgoulhon
- Status changed from needs_work to needs_review
comment:43 Changed 9 months ago by egourgoulhon
- Description modified (diff)
comment:44 in reply to: ↑ 41 ; follow-up: ↓ 45 Changed 8 months ago by tscrim
Finally, some time to do more review. It's looking really good.
Replying to egourgoulhon:
Replying to tscrim:
- Do we need the class FormattedExpansion? It doesn't seem to be used.
Yes, we need it: it is used by the method view() of classes FreeModuleTensor and FreeModuleAltForm, to have a nice output, either text formatted or LaTeX formatted (notebook).
Ah, I see (I never really use the notebook).
- Should we make the morphisms inherit from Morphism and/or have their parents be some kind of Homset?
This is done in the new commit. It introduces free module homomorphisms, via
- the parent class FreeModuleHomset (subclass of sage.categories.homset.Homset)
- the element class FiniteRankFreeModuleMorphism (subclass of sage.categories.morphism.Morphism)
Coercions to/from type-(1,1) tensors have been introduced as well.
I'm thinking we should combine the *morphismTensor into the *morphism methods and the make FreeModuleTensor a base class of FreeModuleHomset. This should fix the need to skip part of the test suite for those classes. Or did you get the dreaded "layout conflict" error?
but within that method, could we also check transitivity (there's a method which does this already, but I forget what it's called)?
Not done yet. Note however that transitivity is implemented in the method FreeModuleTensor.common_basis().
I've had two thoughts about this. The first is pulling out the functionality into a DiGraph instance where the vertices are bases and the edges are the various transition maps, thus you could use the shorted_path method. Also suppose you have a chain A -> B -> C -> D, along the way you would need to compute A -> C, and I'm thinking we should store that morphism (or leave it as an optional argument). Note that you might have to make the transition matrices immutable.
- In Sage, the view function outputs as a pdf. So the name of the view() method is confusing to me. Do you think we could change the name to something else like expand?
We thought quite a lot about the name of this method; here we really need a short name, because it is used very often in interactive mode (notebook). The idea is to display tensors in a nice human readable form, but not to change their state, which something like expand might suggest. display was too long. An alternative was show but we eliminated it because in Sage it is used for graphic outputs. So the short name that we ended with was view. We may also wonder why view should be reserved for pdf output. Wouldn't view_pdf or make_pdf be more suggestive ?
IDK why it was chosen, but for better or worse, that's the current standard. Also the name doesn't quite match what it does IMO. So I'd really like a different name. Perhaps display, print, equation, peq (short for print_equation), ...?
- Fix all failures from the TestSuite or justify why we can skip certain tests.
Done. Some fixes consisted in initializing the objects (e.g. by setting some components) prior to the TestSuite. Note that _test_category had to be skipped when constructing the object by a direct call to __init__, since the correct construction must be performed by a call to parent().element_class. In the latter case, _test_category passed.
These seem to indicate the need for a parent for these elements. For say the linear forms, you should just need a class
LinFormParent(TensorFreeModule): Element = FreeModuleLinForm
(with perhaps a _repr_ stating its the set of all linear forms) and an if statement when constructing the TensorFreeModule. This also means they get the benefits from having a proper category. Do you mind doing this?
Also could you change things like
"blah" + str(obj) + "foo"
to
"blah {} foo".format(obj)
(or if you prefer "blah %s foo"%(obj,))? I believe this is more pythonic, and it makes it a lot easier to read. Thanks.
comment:45 in reply to: ↑ 44 ; follow-up: ↓ 48 Changed 8 months ago by egourgoulhon
Replying to tscrim:
Finally, some time to do more review.
Thank you very much for this new piece of work!
I'm thinking we should combine the *morphismTensor into the *morphism methods and the make FreeModuleTensor a base class of FreeModuleHomset. This should fix the need to skip part of the test suite for those classes. Or did you get the dreaded "layout conflict" error?
Actually, given that endomorphisms are now properly dealt with, as Maps, with some coercion from type-(1,1) tensors, I am wondering about the necessity to maintain the class FreeModuleEndomorphismTensor...
Not done yet. Note however that transitivity is implemented in the method FreeModuleTensor.common_basis().
I've had two thoughts about this. The first is pulling out the functionality into a DiGraph instance where the vertices are bases and the edges are the various transition maps, thus you could use the shorted_path method. Also suppose you have a chain A -> B -> C -> D, along the way you would need to compute A -> C, and I'm thinking we should store that morphism (or leave it as an optional argument). Note that you might have to make the transition matrices immutable.
Using digraphs for this sounds indeed appealing...
- In Sage, the view function outputs as a pdf. So the name of the view() method is confusing to me. Do you think we could change the name to something else like expand?
We thought quite a lot about the name of this method; here we really need a short name, because it is used very often in interactive mode (notebook). The idea is to display tensors in a nice human readable form, but not to change their state, which something like expand might suggest. display was too long. An alternative was show but we eliminated it because in Sage it is used for graphic outputs. So the short name that we ended with was view. We may also wonder why view should be reserved for pdf output. Wouldn't view_pdf or make_pdf be more suggestive ?
IDK why it was chosen, but for better or worse, that's the current standard. Also the name doesn't quite match what it does IMO. So I'd really like a different name. Perhaps display, print, equation, peq (short for print_equation), ...?
OK, maybe display is a good replacement of view. It is longer to write, but one can always use TAB completion.
- Fix all failures from the TestSuite or justify why we can skip certain tests.
Done. Some fixes consisted in initializing the objects (e.g. by setting some components) prior to the TestSuite. Note that _test_category had to be skipped when constructing the object by a direct call to __init__, since the correct construction must be performed by a call to parent().element_class. In the latter case, _test_category passed.
These seem to indicate the need for a parent for these elements.
Not always, if I understand correctly. For elements that have a proper parent, like FreeModuleTensor, the construction has to be performed by parent.element_class, not by a direct call to FreeModuleTensor.__init__, for _test_category to be passed (cf. the doctest of FreeModuleTensor.__init__). If I am correct, this is a general feature of Sage's category framework.
But for elements that are implemented as a subclass of FreeModuleTensor, like FreeModuleLinForm, you are right: a proper parent is missing (see below).
For say the linear forms, you should just need a class
LinFormParent(TensorFreeModule): Element = FreeModuleLinForm(with perhaps a _repr_ stating its the set of all linear forms) and an if statement when constructing the TensorFreeModule. This also means they get the benefits from having a proper category. Do you mind doing this?
Yes I will try. For FreeModuleLinForm there should be no problem. However for FreeModuleAltForm, it seems we are facing the element subclass issue discussed in this thread. Indeed, not all type-(0,p) tensors are alternating forms. Suppose one wants to compute A+B, with A an alternating p-form and B a generic type-(0,p) tensor. Then A and B will have different parents, so that A._add_ cannot be called directly. Of course, one could implement coercions from alternating p-forms to type-(0,p) tensors, but this involves creating a new object from scratch, A' say, by copying the components of A. For objects with a lot of components, in many different bases, this may be not very efficient...
Also could you change things like
"blah" + str(obj) + "foo"to
"blah {} foo".format(obj)(or if you prefer "blah %s foo"%(obj,))? I believe this is more pythonic, and it makes it a lot easier to read. Thanks.
OK, I will. Btw, besides being more pythonic, is there any technical advantage in writing
"blah {} foo".format(obj) instead of "blah" + str(obj) + "foo"? IMHO, the latter is more readable than the former.
comment:46 follow-up: ↓ 51 Changed 8 months ago by jdemeyer
- Milestone changed from sage-6.4 to sage-6.5
- Status changed from needs_review to needs_work
- Work issues set to documentation
The documentation for FiniteRankFreeModule should really explain what's the difference between FiniteRankFreeModule(ZZ, 3) and FreeModule(ZZ, 3). This documentation should be understandable by somebody who has never looked at this ticket.
comment:47 follow-up: ↓ 52 Changed 8 months ago by jdemeyer
You write
The class :class:`FiniteRankFreeModule` does not inherit from :class:`~sage.modules.free_module.FreeModule_generic` since the latter is a derived class of :class:`~sage.modules.module.Module_old`, which does not conform to the new coercion model.
but that's no longer true given #10513.
comment:48 in reply to: ↑ 45 Changed 8 months ago by tscrim
Replying to egourgoulhon:
I'm thinking we should combine the *morphismTensor into the *morphism methods and the make FreeModuleTensor a base class of FreeModuleHomset. This should fix the need to skip part of the test suite for those classes. Or did you get the dreaded "layout conflict" error?
Actually, given that endomorphisms are now properly dealt with, as Maps, with some coercion from type-(1,1) tensors, I am wondering about the necessity to maintain the class FreeModuleEndomorphismTensor...
We're on the same wavelength. Hopefully there won't be a (very annoying) technical issue with python being unable to figure out the MRO.
Using digraphs for this sounds indeed appealing...
Let me know if you have any questions.
OK, maybe display is a good replacement of view. It is longer to write, but one can always use TAB completion.
The more I think about it, the more I like display. Thanks for changing it.
These seem to indicate the need for a parent for these elements.
Not always, if I understand correctly. For elements that have a proper parent, like FreeModuleTensor, the construction has to be performed by parent.element_class, not by a direct call to FreeModuleTensor.__init__, for _test_category to be passed (cf. the doctest of FreeModuleTensor.__init__). If I am correct, this is a general feature of Sage's category framework.
But for elements that are implemented as a subclass of FreeModuleTensor, like FreeModuleLinForm, you are right: a proper parent is missing (see below).
For say the linear forms, you should just need a class
LinFormParent(TensorFreeModule): Element = FreeModuleLinForm(with perhaps a _repr_ stating its the set of all linear forms) and an if statement when constructing the TensorFreeModule. This also means they get the benefits from having a proper category. Do you mind doing this?
Yes I will try. For FreeModuleLinForm there should be no problem. However for FreeModuleAltForm, it seems we are facing the element subclass issue discussed in this thread. Indeed, not all type-(0,p) tensors are alternating forms. Suppose one wants to compute A+B, with A an alternating p-form and B a generic type-(0,p) tensor. Then A and B will have different parents, so that A._add_ cannot be called directly. Of course, one could implement coercions from alternating p-forms to type-(0,p) tensors, but this involves creating a new object from scratch, A' say, by copying the components of A. For objects with a lot of components, in many different bases, this may be not very efficient...
I guess the question is how often do you expect to do this? Plus I suspect a lot of the data can just be referenced, not copied, since the data is (essentially) immutable. It's not much different than other parts of Sage and having to be careful about parents. For instance:
sage: %timeit ZZ(1) + QQ(1) 100000 loops, best of 3: 10.4 µs per loop sage: %timeit ZZ(1) + QQ(1) 100000 loops, best of 3: 9.73 µs per loop sage: %timeit QQ(1) + QQ(1) 100000 loops, best of 3: 6.53 µs per loop sage: %timeit QQ(1) + QQ(1) 100000 loops, best of 3: 7.22 µs per loop
The issue is to minimize the penalty. A benefit from doing the implementation is you can add two alternating forms and get an alternating form (for free I believe).
Also could you change things like
"blah" + str(obj) + "foo"to
"blah {} foo".format(obj)(or if you prefer "blah %s foo"%(obj,))? I believe this is more pythonic, and it makes it a lot easier to read. Thanks.
OK, I will. Btw, besides being more pythonic, is there any technical advantage in writing
"blah {} foo".format(obj) instead of "blah" + str(obj) + "foo"? IMHO, the latter is more readable than the former.
The "blah %s foo"%obj is the fastest (not by too much), but if obj is a tuple, an error gets raised (the wrapping (obj,) gets rid of this for marginal penalty). The format allows you to do fancier things like repeating objects, for some time, the % syntax was deprecated in python3 for format (again, not anymore), and it's on the same order of speed as using +'s. So really there's no real technical advantage other than readability (all python code I've previously seen used the % and more recently format, but to be honest, recent python code has all be Sage).
Somewhat unrelated, I do miss the C++ cout<<"blah"<<obj<<"foo"...
Last I agree with Jeroen, especially comment:47.
Thanks again for your work on all of this.
comment:49 Changed 7 months ago by git
- Commit changed from 6e53a8d3c92d405541e2a21dff5a867d402937e6 to 968468796989dd7415c235a6895dadcab6d9e108
Branch pushed to git repo; I updated commit sha1. New commits:
9684687 | Add parents for free module automorphisms and alternating forms; improve documentation. |
comment:50 Changed 7 months ago by egourgoulhon
Thank you very much Travis and Jeroen for your comments.
I've just committed the following changes:
- Introduced the parent class ExtPowerFreeModule for alternating forms of a given degree on a free module. All alternating forms are now described by the element class FreeModuleAltForm. In particular, the class FreeModuleLinForm has been suppressed.
- Introduced the parent class FreeModuleLinearGroup for the general linear group GL(M) of a free module M. The corresponding element class is FreeModuleAutomorphism, which has been entirely rewritten (it is now both a group element class and a class of type-(1,1) tensors).
- Added the following coercions:
- alternating forms of degree p ---> tensors of type (0,p)
- tensors of type (0,1) ---> alternating forms of degree 1 (linear form)
- automorphisms ---> tensors of type (1,1)
- automorphisms ---> endomorphisms
- Added the following conversions:
- (invertible) endomorphisms ---> automorphisms
- (invertible) type-(1,1) tensors ---> automorphisms
- Suppressed the classes FreeModuleAutomorphismTensor and FreeModuleIdentityTensor, which were no longer needed thanks to the above coercions.
- Suppressed the class FreeModuleEndomorphismTensor, which was no longer needed thanks to the coercions endomorphisms <---> tensors of type (1,1).
- Renamed method view() to display(); view() is still there (it is heavily used by the users of SageManifolds) but it is set to deprecated.
- Implemented transitivity in FiniteRankFreeModule.change_of_basis(), i.e. if the changes of basis A->B and B->C are known and the change of basis A->C is required, it is computed by automorphism composition.
- Added argument from_family in FiniteRankFreeModule.basis(), thereby allowing to construct a new basis from a family of linearly independent module elements (previously, one had to pass a previous basis and an explicit automorphism relating it to the new basis).
- Systematic use of "...{}...".format(...) instead of "..." + str(...) + "..." (except in LaTeX strings, because of the {})
- Reorganized documentation files in src/doc/en/reference/tensor_free_modules/*.rst
- Added comparisons
- between FiniteRankFreeModule and FreeModule,
- between FiniteRankFreeModule and VectorSpace,
- between FiniteRankFreeModule and CombinatorialFreeModule
in the documentation of FiniteRankFreeModule. You can see the result here. I've also taken into account #comment:47.
All test suites (both for parents and elements) are passed.
comment:51 in reply to: ↑ 46 Changed 7 months ago by egourgoulhon
Replying to jdemeyer:
The documentation for FiniteRankFreeModule should really explain what's the difference between FiniteRankFreeModule(ZZ, 3) and FreeModule(ZZ, 3). This documentation should be understandable by somebody who has never looked at this ticket.
This is done in the new commit: see here.
comment:52 in reply to: ↑ 47 Changed 7 months ago by egourgoulhon
Replying to jdemeyer:
You write
The class :class:`FiniteRankFreeModule` does not inherit from :class:`~sage.modules.free_module.FreeModule_generic` since the latter is a derived class of :class:`~sage.modules.module.Module_old`, which does not conform to the new coercion model.but that's no longer true given #10513.
Thanks for pointing this. I've modified the documentation of FiniteRankFreeModule accordingly, see here.
comment:53 Changed 7 months ago by egourgoulhon
- Status changed from needs_work to needs_review
- Work issues documentation deleted
comment:54 Changed 7 months ago by egourgoulhon
- Description modified (diff)
comment:55 Changed 6 months ago by git
- Commit changed from 968468796989dd7415c235a6895dadcab6d9e108 to 278c94a66f826eb9dfbf5399441fcfda6f883641
Branch pushed to git repo; I updated commit sha1. New commits:
278c94a | Minor changes to coincide with version 0.7 of SageManifolds |
comment:56 Changed 6 months ago by egourgoulhon
The new commit makes the ticket coincide with the algebraic part of SageManifolds 0.7, which was released yesterday. The changes are very tiny with respect to commit 9684687; here is the log:
- src/sage/tensor/modules/comp.py: zero_element() --> zero() (zero_element() being deprecated in Sage)
- src/sage/tensor/modules/format_utilities.py: improve documentation
- class FormattedExpansion:
- suppress the attribute "tensor" (it was actually useless)
- rename attribute "txt" to "_txt"
- rename attribute "latex" to "_latex"
- class FiniteRankFreeModule: method identity_map(): add arguments "name" and "latex_name"
- class FreeModuleHomset: add method one() (for endomorphism sets)
- class FreeModuleTensor: suppress methods __radd__() and __rsub__() : they were introduced to write 0 + t and 0 - t, where t is a tensor, before the coercion of 0 to a tensor was implemented.
- src/doc/en/reference/tensor_free_modules/index.rst:add html target ".. _tensors-on-free-modules"
comment:57 Changed 6 months ago by git
- Commit changed from 278c94a66f826eb9dfbf5399441fcfda6f883641 to af361f7b69bff3dc3dc24edbe8e3958ce51255bb
Branch pushed to git repo; I updated commit sha1. New commits:
af361f7 | Fix a merge conflict in the reference manual w.r.t. the latest develop version of Sage |
comment:58 Changed 6 months ago by git
- Commit changed from af361f7b69bff3dc3dc24edbe8e3958ce51255bb to 8f1134352dcd79cf1b3e087dcb30bd023470ec1d
Branch pushed to git repo; I updated commit sha1. New commits:
8f11343 | Merge #15916 into Sage 6.6.beta5 to solve a conflict in src/doc/en/reference/index.rst |
comment:59 Changed 6 months ago by tscrim
- Keywords days64 added
- Status changed from needs_review to positive_review
I'm happy with the current version of this, so positive review. Thanks for your work on this!
comment:60 Changed 6 months ago by vbraun
- Status changed from positive_review to needs_work
Doctests fail
comment:61 Changed 6 months ago by git
- Commit changed from 8f1134352dcd79cf1b3e087dcb30bd023470ec1d to 99f9ac53bd3e42dd00f5d05b7b126a632161ca55
Branch pushed to git repo; I updated commit sha1. New commits:
99f9ac5 | Fix doctest because of new doc category. |
comment:62 follow-up: ↓ 63 Changed 6 months ago by tscrim
- Status changed from needs_work to positive_review
This is the only (trivial) failure I can see.
comment:63 in reply to: ↑ 62 ; follow-up: ↓ 64 Changed 6 months ago by egourgoulhon
Replying to tscrim:
This is the only (trivial) failure I can see.
In the patchbot log, there are many doctest failures but they pertain to other parts of Sage: all doctests of files in src/sage/tensor/modules (i.e. the sources of this ticket) pass. So I am puzzled: why the patchbot returns so many failures? Could it be that the ticket branch is based on Sage 6.6.beta5?
comment:64 in reply to: ↑ 63 ; follow-up: ↓ 65 Changed 6 months ago by tscrim
Replying to egourgoulhon:
Replying to tscrim:
This is the only (trivial) failure I can see.
In the patchbot log, there are many doctest failures but they pertain to other parts of Sage: all doctests of files in src/sage/tensor/modules (i.e. the sources of this ticket) pass. So I am puzzled: why the patchbot returns so many failures? Could it be that the ticket branch is based on Sage 6.6.beta5?
It has to do with file permissions on the patchbot account. They are harmless and not really errors as far as Sage is concerned, but comes from python IIRC.
comment:65 in reply to: ↑ 64 Changed 6 months ago by egourgoulhon
Replying to tscrim:
Replying to egourgoulhon:
Replying to tscrim:
This is the only (trivial) failure I can see.
In the patchbot log, there are many doctest failures but they pertain to other parts of Sage: all doctests of files in src/sage/tensor/modules (i.e. the sources of this ticket) pass. So I am puzzled: why the patchbot returns so many failures? Could it be that the ticket branch is based on Sage 6.6.beta5?
It has to do with file permissions on the patchbot account. They are harmless and not really errors as far as Sage is concerned, but comes from python IIRC.
OK I see. Thank you Travis!
comment:66 Changed 6 months ago by vbraun
- Branch changed from public/tensor_modules-15916 to 99f9ac53bd3e42dd00f5d05b7b126a632161ca55
- Resolution set to fixed
- Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits: