Opened 17 months ago
Closed 15 months ago
#28787 closed enhancement (fixed)
_add_ method for tensors with indices
Reported by: | gh-LBrunswic | Owned by: | gh-LBrunswic |
---|---|---|---|
Priority: | minor | Milestone: | sage-9.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 17 months ago by
- Branch set to u/gh-LBrunswic/_add__method_for_tensors_with_indices
comment:2 Changed 17 months ago by
- Commit set to 232ae1bddc3535970d409c448d4ff26dddc47e8d
- Owner changed from (none) to gh-LBrunswic
comment:3 Changed 17 months ago by
- Commit changed from 232ae1bddc3535970d409c448d4ff26dddc47e8d to 8fc152a3f1df0531a2b4580f7b704e63db2a9a57
comment:4 Changed 17 months 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_contractions-28784' into t/28787/_add__method_for_tensors_with_indices
|
comment:5 Changed 17 months ago by
- Commit changed from 30b92a8a62bb4fccc1a996e7a2c57347f7752cfa to 6f9c651d81094bb9ab043052ce324e0fdb69563c
comment:6 Changed 17 months 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 17 months 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 17 months ago by
- Status changed from new to needs_review
comment:9 Changed 17 months 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 17 months 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 17 months 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 follow-up: ↓ 13 Changed 17 months 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 17 months ago by
Replying to gh-LBrunswic:
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 17 months 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 17 months 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 17 months 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 17 months 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 17 months 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 17 months 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 17 months ago by
- Branch changed from u/gh-LBrunswic/_add__method_for_tensors_with_indices to public/manifolds/add_tensor_with_indices-28787
- 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 17 months ago by
PS: to see my changes, simply click on the blue 8fcd437
above.
comment:22 Changed 17 months 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 follow-up: ↓ 24 Changed 17 months 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 ; follow-up: ↓ 25 Changed 17 months ago by
Replying to gh-LBrunswic:
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 17 months ago by
Replying to gh-LBrunswic:
Replying to gh-LBrunswic:
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 17 months 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 17 months 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 17 months 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 17 months 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 17 months ago by
- Status changed from needs_review to needs_work
comment:31 Changed 17 months 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 17 months ago by
- Status changed from needs_work to needs_review
comment:33 Changed 17 months 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 17 months 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 17 months ago by
typo "Decompostion"
comment:36 Changed 17 months 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 17 months 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 17 months 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 follow-up: ↓ 40 Changed 17 months 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 17 months ago by
Replying to gh-LBrunswic:
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 16 months 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 0-indexed notations. Therefore, one would need to translate a permutation acting on [0,...,n-1] 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 16 months ago by
- Commit changed from aaf530ccfffbfb80f34164226bc8b16865e5eeeb to cd655b18ba9fb5f8fab0cfaf083b29b0cffaa4c6
comment:43 Changed 16 months ago by
- Milestone changed from sage-9.0 to sage-9.1
Ticket retargeted after milestone closed
comment:44 Changed 15 months ago by
May I ask about the status of this ticket? Is it again ready for review?
comment:45 Changed 15 months 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 15 months 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_indices-28787
|
comment:47 Changed 15 months ago by
It seems okay to me, I thought I had asked for review after my previous commit.
comment:48 Changed 15 months ago by
- Status changed from needs_work to needs_review
comment:49 Changed 15 months 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 15 months 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 15 months 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 15 months 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 15 months 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 15 months 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 15 months 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/site-packages/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 15 months 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 15 months ago by
- Status changed from needs_work to needs_review
comment:58 Changed 15 months ago by
- Status changed from needs_review to positive_review
comment:59 Changed 15 months ago by
- Branch changed from public/manifolds/add_tensor_with_indices-28787 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