Opened 2 years ago
Closed 2 years ago
#28787 closed enhancement (fixed)
_add_ method for tensors with indices
Reported by:  ghLBrunswic  Owned by:  ghLBrunswic 

Priority:  minor  Milestone:  sage9.1 
Component:  linear algebra  Keywords:  tensor, contraction, symmetries, tensor with indices, manifolds 
Cc:  egourgoulhon  Merged in:  
Authors:  Léo Brunswic  Reviewers:  Eric Gourgoulhon, Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  a1b9630 (Commits, GitHub, GitLab)  Commit:  a1b963050be2f849e23d84cb0d74c6c8fcd45009 
Dependencies:  Stopgaps: 
Description
One may want to add/contract tensors with indices in the following way :
T["^ijklm_m]+T["^mklij_m"]
This entails to implement
 _get_item_ method
 _set_item_ method
 _add_ method
which all keep track of the indices names.
Change History (59)
comment:1 Changed 2 years ago by
 Branch set to u/ghLBrunswic/_add__method_for_tensors_with_indices
comment:2 Changed 2 years ago by
 Commit set to 232ae1bddc3535970d409c448d4ff26dddc47e8d
 Owner changed from (none) to ghLBrunswic
comment:3 Changed 2 years ago by
 Commit changed from 232ae1bddc3535970d409c448d4ff26dddc47e8d to 8fc152a3f1df0531a2b4580f7b704e63db2a9a57
comment:4 Changed 2 years ago by
 Commit changed from 8fc152a3f1df0531a2b4580f7b704e63db2a9a57 to 30b92a8a62bb4fccc1a996e7a2c57347f7752cfa
Branch pushed to git repo; I updated commit sha1. New commits:
1d1e4ea  Implementation of multiple symmetries for tensor with indices as well as convention checks

59c86e3  Adding check to raise exception on repeated indices of same type. Implementation of multiple contractions

4b9a565  The convention without leading '^' for contravariant indices is now accepted as is used to. A regex corrected. doctest added to illustrate new functionalities.

0823e29  Minor corrections for #28784

3013354  Marginal modifications

30b92a8  Merge branch 't/28784/public/manifolds/tensor_multiple_symmetries_contractions28784' into t/28787/_add__method_for_tensors_with_indices

comment:5 Changed 2 years ago by
 Commit changed from 30b92a8a62bb4fccc1a996e7a2c57347f7752cfa to 6f9c651d81094bb9ab043052ce324e0fdb69563c
comment:6 Changed 2 years ago by
 Commit changed from 6f9c651d81094bb9ab043052ce324e0fdb69563c to 8e29f7731bf5daba06f16f7c59eb0047d81ec36c
Branch pushed to git repo; I updated commit sha1. New commits:
8e29f77  _parse_indices staticmethod added ; new method permute_indices added ; _getitem_ and _set_item_ added ; code factorisation of __init__ and __add__ using _parse_indices and permute_indices.

comment:7 Changed 2 years ago by
 Commit changed from 8e29f7731bf5daba06f16f7c59eb0047d81ec36c to 3506466435524e1e971a1403845cb4db3452a45e
Branch pushed to git repo; I updated commit sha1. New commits:
3506466  Many error corrected. Added doctest

comment:8 Changed 2 years ago by
 Status changed from new to needs_review
comment:9 Changed 2 years ago by
 Commit changed from 3506466435524e1e971a1403845cb4db3452a45e to 228824989acbe18d43cf053cb0520749df893389
Branch pushed to git repo; I updated commit sha1. New commits:
2288249  Some PEP8 enforcement

comment:10 Changed 2 years ago by
Thanks for providing this functionality! The code looks nice. However, as revealed by the patchbot, there are a few minor issues:
 running
sage coverage src/sage/tensor/modules/tensor_with_indices.py
reveals incomplete coverage SymmetricGroup
is imported but not used there are invalid escape sequences in the lines
if re.search("[()\[\]]",con) is not None: raise ValueError("No symmetry allowed") if re.search("[()\[\]]",cov) is not None: raise ValueError("No symmetry allowed")
Most probably, you should prefix the string"[()\[\]]"
withr
to make it a raw string. Side remark: the error message passed to exceptions should start with a lower case letter; so please change"No symmetry allowed"
to"no symmetry allowed"
 Building the documentation via
sage docbuild reference/tensor_free_modules html
yields some errors, two of them being related to the characters\]
and\[
above and should be fixed with ther
prefix
comment:11 Changed 2 years ago by
 Commit changed from 228824989acbe18d43cf053cb0520749df893389 to e44197b6dec0ae04a3e8b83ae64965a519136bdb
Branch pushed to git repo; I updated commit sha1. New commits:
e44197b  raw prefix 'r' added before regex. Unused import deleted. full doc Coverage.docbuild errors corrected.

comment:12 followup: ↓ 13 Changed 2 years ago by
Thx for the review. I'm sorry, I'm still familiarizing myself with the tools involved in the review process.
comment:13 in reply to: ↑ 12 Changed 2 years ago by
Replying to ghLBrunswic:
Thx for the review. I'm sorry, I'm still familiarizing myself with the tools involved in the review process.
No problem. I fully understand this of course. To get access to the patchbot reports, click on the button with the Sage version in the top right of the ticket description panel.
comment:14 Changed 2 years ago by
 Commit changed from e44197b6dec0ae04a3e8b83ae64965a519136bdb to b34a712d979ee04d25c5d8b091f00835f7252c4d
Branch pushed to git repo; I updated commit sha1. New commits:
b34a712  Some docbuild error corrected

comment:15 Changed 2 years ago by
 Commit changed from b34a712d979ee04d25c5d8b091f00835f7252c4d to 0fafb7804d4c289f18aeebbb50eebcf54c3b7de6
Branch pushed to git repo; I updated commit sha1. New commits:
0fafb78  Some docbuild error corrected

comment:16 Changed 2 years ago by
 Commit changed from 0fafb7804d4c289f18aeebbb50eebcf54c3b7de6 to 831b050f3bf23a9068538be51f1b97c06fa9aa7c
Branch pushed to git repo; I updated commit sha1. New commits:
831b050  Merge branch 'develop' into t/28787/_add__method_for_tensors_with_indices

comment:17 Changed 2 years ago by
 Commit changed from 831b050f3bf23a9068538be51f1b97c06fa9aa7c to 35032fa8f279f33e22746203184272e101ce83cc
Branch pushed to git repo; I updated commit sha1. New commits:
35032fa  corrected a plugin error

comment:18 Changed 2 years ago by
 Commit changed from 35032fa8f279f33e22746203184272e101ce83cc to a972fe1d843b83fa4329b89241239573a9c981a8
Branch pushed to git repo; I updated commit sha1. New commits:
a972fe1  corrected a plugin error

comment:19 Changed 2 years ago by
Sometime the plugin is wrong:
 EXAMPLES: + EXAMPLES::
was a wrong move, because this is not followed by doctests, but by more text..
comment:20 Changed 2 years ago by
 Branch changed from u/ghLBrunswic/_add__method_for_tensors_with_indices to public/manifolds/add_tensor_with_indices28787
 Commit changed from a972fe1d843b83fa4329b89241239573a9c981a8 to 8fcd4372cb235c4661c77351fba43f02e9a34e90
 Status changed from needs_review to needs_work
I've performed some corrections to the docstrings, including that mentioned by Frédéric (comment:19). Now the documentation should build successfully. Note that some of the changes are mere deletions of white space characters on empty lines or at the end of lines (you should configure your editor to do that automatically when saving the file to disk).
The documentation of the new functionalities added by this ticket does not appear in the html output (i.e. in local/share/doc/sage/html/en/reference/tensor_free_modules/sage/tensor/modules/tensor_with_indices.html
) because the doc of underscored methods, such as __add__
, is not generated. You should therefore add relevant examples in the main docstring of TensorWithIndices
.
New commits:
8fcd437  Minor fixes in the documentation of tensors with indices (trac #28787)

comment:21 Changed 2 years ago by
PS: to see my changes, simply click on the blue 8fcd437
above.
comment:22 Changed 2 years ago by
 Status changed from needs_work to needs_review
Setting the ticket back to needs_review for the patchbot to run on it.
comment:23 followup: ↓ 24 Changed 2 years ago by
Thanks a lot, I spent an unreasonable amount of time yesterday trying to understand what was the problem.
I will consider putting some time into patchbot to see how one could improve on this. Trying to understand the issue I've already read some of the code of patchbot.
comment:24 in reply to: ↑ 23 ; followup: ↓ 25 Changed 2 years ago by
Replying to ghLBrunswic:
Thanks a lot, I spent an unreasonable amount of time yesterday trying to understand what was the problem.
The worst part being that I was actually able to build sagemath with the doc without errors. Only patchbot was raising errors...
I will consider putting some time into patchbot to see how one could improve on this. Trying to understand the issue I've already read some of the code of patchbot.
comment:25 in reply to: ↑ 24 Changed 2 years ago by
Replying to ghLBrunswic:
Replying to ghLBrunswic:
Thanks a lot, I spent an unreasonable amount of time yesterday trying to understand what was the problem.
The worst part being that I was actually able to build sagemath with the doc without errors. Only patchbot was raising errors...
Actually, the patchbot reports are to be taken with a grain of salt. A human eye has always to take a look. Apart from the EXAMPLE:
case pointed by Frédéric (maybe in that case the patchbot error was triggered by EXAMPLE:
not being followed by an empty line), another case is some spurious report of unused import (see e.g. this example).
I will consider putting some time into patchbot to see how one could improve on this.
Thanks!
comment:26 Changed 2 years ago by
 Commit changed from 8fcd4372cb235c4661c77351fba43f02e9a34e90 to 5400bf73b356187d08d242f05da01860f2c43fe3
Branch pushed to git repo; I updated commit sha1. New commits:
5400bf7  updated doc examples illustrating __add__

comment:27 Changed 2 years ago by
Doctest lines after TESTS:: should be indented, as for EXAMPLES::
+ TESTS:: + + sage: from sage.tensor.modules.tensor_with_indices import TensorWithIndices + sage: TensorWithIndices._parse_indices('([..])') # nested symmetries + Traceback (most recent call last):
The patchbot plugins are not smart, but there is no simple way to make them smarter. When they give a red light, you should judge by yourself if there is an error or not.
comment:28 Changed 2 years ago by
 Commit changed from 5400bf73b356187d08d242f05da01860f2c43fe3 to 3e37ac0ff5939daf098d32417b42959c7df97694
Branch pushed to git repo; I updated commit sha1. New commits:
3e37ac0  indentation corrected

comment:29 Changed 2 years ago by
I'm not critisizing the work that has been put in the patchbot and I realize the work there is more than simply changing a few regex. On the contrary, I do appreciate the work that has been put into the review process of sagemath, which includes the patchbot. I feel that improving on the process (and the documentation) is improving sagemath on the long run.
comment:30 Changed 2 years ago by
 Status changed from needs_review to needs_work
comment:31 Changed 2 years ago by
 Commit changed from 3e37ac0ff5939daf098d32417b42959c7df97694 to 5e7817d0a02ae0861801bdbfcc6d4b179f737fe0
Branch pushed to git repo; I updated commit sha1. New commits:
5e7817d  Doc corrected for __setitem__ and __getitem__

comment:32 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:33 Changed 2 years ago by
 Commit changed from 5e7817d0a02ae0861801bdbfcc6d4b179f737fe0 to a2f2383f8391f43f94eebab146f781e86ecf4695
Branch pushed to git repo; I updated commit sha1. New commits:
a2f2383  Doc corrected (again) for __setitem__ and __getitem__

comment:34 Changed 2 years ago by
 Commit changed from a2f2383f8391f43f94eebab146f781e86ecf4695 to 4dd4a6d61e87c21728b6aa6bd1c5f9dcea563058
Branch pushed to git repo; I updated commit sha1. New commits:
4dd4a6d  PEP enforcement

comment:35 Changed 2 years ago by
typo "Decompostion"
comment:36 Changed 2 years ago by
 Commit changed from 4dd4a6d61e87c21728b6aa6bd1c5f9dcea563058 to 1904d92d2c8b8754d43dc6e6a6697bd76002f7a5
Branch pushed to git repo; I updated commit sha1. New commits:
1904d92  Typo correction

comment:37 Changed 2 years ago by
 Commit changed from 1904d92d2c8b8754d43dc6e6a6697bd76002f7a5 to aaf530ccfffbfb80f34164226bc8b16865e5eeeb
Branch pushed to git repo; I updated commit sha1. New commits:
aaf530c  #28787: small reviewer tweaks

comment:38 Changed 2 years ago by
I've just pushed a new commit, with very minor tweaks (mostly indentation in _parse_indices()
docstring).
A last point: could you please document further the argument permutation
of the method permute_indices()
, in particular stating in which format it is expected, possibly providing more examples?
comment:39 followup: ↓ 40 Changed 2 years ago by
 Status changed from needs_review to needs_work
I then will add some overloading to accept a permutation given by an element of a symmetric group. This seems natural.
comment:40 in reply to: ↑ 39 Changed 2 years ago by
Replying to ghLBrunswic:
I then will add some overloading to accept a permutation given by an element of a symmetric group. This seems natural.
I was not asking that much, but simply that you document permutation
in the INPUT
section by something like "tuple/list of index positions describing the permutation...". But I agree it is quite natural to pass an element of a symmetric group as well.
comment:41 Changed 2 years ago by
I struggled with adding more general permutation for several reasons :
 The word problem is only implemented for permutation group acting on [1,...,n] while we more commonly use 0indexed notations. Therefore, one would need to translate a permutation acting on [0,...,n1] into a permutation acting on [1,...,n]. While this is not difficult, it seems particularly unnatural : such conversion should be done in the Permutation group module.
 The overloading would depends on the overloading of PermutationGroupElement?
Therefore, I think it's better to postpone this supplementaty functionality.
comment:42 Changed 2 years ago by
 Commit changed from aaf530ccfffbfb80f34164226bc8b16865e5eeeb to cd655b18ba9fb5f8fab0cfaf083b29b0cffaa4c6
comment:43 Changed 2 years ago by
 Milestone changed from sage9.0 to sage9.1
Ticket retargeted after milestone closed
comment:44 Changed 2 years ago by
May I ask about the status of this ticket? Is it again ready for review?
comment:45 Changed 2 years ago by
I haven't taken the time rework on it since the holydays (and the launch of sage9). I have to go through it again.
comment:46 Changed 2 years ago by
 Commit changed from cd655b18ba9fb5f8fab0cfaf083b29b0cffaa4c6 to 4340fc877393abc5491f9ceae2ed74c7c4d77b2f
Branch pushed to git repo; I updated commit sha1. New commits:
4340fc8  Merge branch 'develop' into t/28787/public/manifolds/add_tensor_with_indices28787

comment:47 Changed 2 years ago by
It seems okay to me, I thought I had asked for review after my previous commit.
comment:48 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:49 Changed 2 years ago by
It looks good to me, except that the patchbot "blocks" plugin reports this error.
This plugin forbids "Returns" at the start of a new line, see rule no. 6 in line 581 of the plugin source code. This is because, according to PEP 257, "The docstring is a phrase ending in a period. It prescribes the function or method's effect as a command ("Do this", "Return that"), not as a description; e.g. don't write "Returns the pathname ...". So the doctstring of the method _parse_indices
should start with "Parse", not "Parses", etc. Could you fix this please?
comment:50 Changed 2 years ago by
This is likely a doc problem:
 ``permutation``  permutation that has to be applied to the indices  the input should be a ``list`` containing the second line of the permutation  in Cauchy notation. + the input should be a ``list`` containing the second line of the permutation + in Cauchy notation
comment:51 Changed 2 years ago by
 Commit changed from 4340fc877393abc5491f9ceae2ed74c7c4d77b2f to e5882893d2f9029b3b58e3fb323dbbf04f8fce46
Branch pushed to git repo; I updated commit sha1. New commits:
e588289  Docstring correction

comment:52 Changed 2 years ago by
 Commit changed from e5882893d2f9029b3b58e3fb323dbbf04f8fce46 to fde92459be55a20e6cdefc230ea107450495f925
Branch pushed to git repo; I updated commit sha1. New commits:
fde9245  Docstring correction

comment:53 Changed 2 years ago by
 Reviewers set to Eric Gourgoulhon, Travis Scrimshaw
 Status changed from needs_review to positive_review
Thanks for the changes. The (relevant) patchbot in now all green.
Thanks for this nice enhancement!
comment:54 Changed 2 years ago by
Thanks! I understand the importance of the constraints on the docstrings but it's plainful to get blocked for a whitespace.
comment:55 Changed 2 years ago by
 Status changed from positive_review to needs_work
On Python 2:
[dochtml] File "/var/lib/buildbot/slave/sage2_git/build/local/lib/python2.7/sitepackages/sage/tensor/modules/tensor_with_indices.py", line 941 [dochtml] [swap(*param, self._tensor.tensor_rank()) for param in swap_params], [dochtml] SyntaxError: only named arguments may follow *expression
comment:56 Changed 2 years ago by
 Commit changed from fde92459be55a20e6cdefc230ea107450495f925 to a1b963050be2f849e23d84cb0d74c6c8fcd45009
Branch pushed to git repo; I updated commit sha1. New commits:
a1b9630  python2 compatibility

comment:57 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:58 Changed 2 years ago by
 Status changed from needs_review to positive_review
comment:59 Changed 2 years ago by
 Branch changed from public/manifolds/add_tensor_with_indices28787 to a1b963050be2f849e23d84cb0d74c6c8fcd45009
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
not yet done