Opened 6 months ago

Closed 4 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) 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 6 months ago by gh-LBrunswic

  • Branch set to u/gh-LBrunswic/_add__method_for_tensors_with_indices

comment:2 Changed 6 months ago by gh-LBrunswic

  • Commit set to 232ae1bddc3535970d409c448d4ff26dddc47e8d
  • Owner changed from (none) to gh-LBrunswic

comment:3 Changed 6 months ago by git

  • Commit changed from 232ae1bddc3535970d409c448d4ff26dddc47e8d to 8fc152a3f1df0531a2b4580f7b704e63db2a9a57

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

8fc152anot yet done

comment:4 Changed 6 months ago by git

  • Commit changed from 8fc152a3f1df0531a2b4580f7b704e63db2a9a57 to 30b92a8a62bb4fccc1a996e7a2c57347f7752cfa

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

1d1e4eaImplementation of multiple symmetries for tensor with indices as well as convention checks
59c86e3Adding check to raise exception on repeated indices of same type. Implementation of multiple contractions
4b9a565The convention without leading '^' for contravariant indices is now accepted as is used to. A regex corrected. doctest added to illustrate new functionalities.
0823e29Minor corrections for #28784
3013354Marginal modifications
30b92a8Merge branch 't/28784/public/manifolds/tensor_multiple_symmetries_contractions-28784' into t/28787/_add__method_for_tensors_with_indices

comment:5 Changed 6 months ago by git

  • Commit changed from 30b92a8a62bb4fccc1a996e7a2c57347f7752cfa to 6f9c651d81094bb9ab043052ce324e0fdb69563c

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

e659567__add__ method completed, doctest added, basic __getitem__ method added, __sub__ method added
6f9c651Correction of the __add__ method to have a more natural indices convention

comment:6 Changed 6 months ago by git

  • 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 6 months ago by git

  • Commit changed from 8e29f7731bf5daba06f16f7c59eb0047d81ec36c to 3506466435524e1e971a1403845cb4db3452a45e

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

3506466Many error corrected. Added doctest

comment:8 Changed 6 months ago by gh-LBrunswic

  • Status changed from new to needs_review

comment:9 Changed 6 months ago by git

  • Commit changed from 3506466435524e1e971a1403845cb4db3452a45e to 228824989acbe18d43cf053cb0520749df893389

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

2288249Some PEP8 enforcement

comment:10 Changed 6 months ago by egourgoulhon

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 "[()\[\]]" with r 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 the r prefix

comment:11 Changed 6 months ago by git

  • Commit changed from 228824989acbe18d43cf053cb0520749df893389 to e44197b6dec0ae04a3e8b83ae64965a519136bdb

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

e44197braw prefix 'r' added before regex. Unused import deleted. full doc Coverage.docbuild errors corrected.

comment:12 follow-up: Changed 6 months ago by gh-LBrunswic

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 6 months ago by egourgoulhon

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 6 months ago by git

  • Commit changed from e44197b6dec0ae04a3e8b83ae64965a519136bdb to b34a712d979ee04d25c5d8b091f00835f7252c4d

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

b34a712Some docbuild error corrected

comment:15 Changed 6 months ago by git

  • Commit changed from b34a712d979ee04d25c5d8b091f00835f7252c4d to 0fafb7804d4c289f18aeebbb50eebcf54c3b7de6

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

0fafb78Some docbuild error corrected

comment:16 Changed 6 months ago by git

  • Commit changed from 0fafb7804d4c289f18aeebbb50eebcf54c3b7de6 to 831b050f3bf23a9068538be51f1b97c06fa9aa7c

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

831b050Merge branch 'develop' into t/28787/_add__method_for_tensors_with_indices

comment:17 Changed 6 months ago by git

  • Commit changed from 831b050f3bf23a9068538be51f1b97c06fa9aa7c to 35032fa8f279f33e22746203184272e101ce83cc

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

35032facorrected a plugin error

comment:18 Changed 6 months ago by git

  • Commit changed from 35032fa8f279f33e22746203184272e101ce83cc to a972fe1d843b83fa4329b89241239573a9c981a8

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

a972fe1corrected a plugin error

comment:19 Changed 6 months ago by chapoton

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 6 months ago by egourgoulhon

  • 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:

8fcd437Minor fixes in the documentation of tensors with indices (trac #28787)

comment:21 Changed 6 months ago by egourgoulhon

PS: to see my changes, simply click on the blue 8fcd437 above.

comment:22 Changed 6 months ago by egourgoulhon

  • 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: Changed 6 months ago by gh-LBrunswic

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: Changed 6 months ago by 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...

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 6 months ago by egourgoulhon

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 6 months ago by git

  • Commit changed from 8fcd4372cb235c4661c77351fba43f02e9a34e90 to 5400bf73b356187d08d242f05da01860f2c43fe3

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

5400bf7updated doc examples illustrating __add__

comment:27 Changed 6 months ago by chapoton

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 6 months ago by git

  • Commit changed from 5400bf73b356187d08d242f05da01860f2c43fe3 to 3e37ac0ff5939daf098d32417b42959c7df97694

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

3e37ac0indentation corrected

comment:29 Changed 6 months ago by gh-LBrunswic

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 6 months ago by gh-LBrunswic

  • Status changed from needs_review to needs_work

comment:31 Changed 6 months ago by git

  • Commit changed from 3e37ac0ff5939daf098d32417b42959c7df97694 to 5e7817d0a02ae0861801bdbfcc6d4b179f737fe0

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

5e7817dDoc corrected for __setitem__ and __getitem__

comment:32 Changed 6 months ago by gh-LBrunswic

  • Status changed from needs_work to needs_review

comment:33 Changed 6 months ago by git

  • Commit changed from 5e7817d0a02ae0861801bdbfcc6d4b179f737fe0 to a2f2383f8391f43f94eebab146f781e86ecf4695

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

a2f2383Doc corrected (again) for __setitem__ and __getitem__

comment:34 Changed 6 months ago by git

  • Commit changed from a2f2383f8391f43f94eebab146f781e86ecf4695 to 4dd4a6d61e87c21728b6aa6bd1c5f9dcea563058

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

4dd4a6dPEP enforcement

comment:35 Changed 6 months ago by chapoton

typo "Decompostion"

comment:36 Changed 6 months ago by git

  • Commit changed from 4dd4a6d61e87c21728b6aa6bd1c5f9dcea563058 to 1904d92d2c8b8754d43dc6e6a6697bd76002f7a5

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

1904d92Typo correction

comment:37 Changed 6 months ago by git

  • 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 6 months ago by egourgoulhon

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: Changed 6 months ago by gh-LBrunswic

  • 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 6 months ago by egourgoulhon

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 6 months ago by gh-LBrunswic

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.

Therefore, I think it's better to postpone this supplementaty functionality.

comment:42 Changed 6 months ago by git

  • Commit changed from aaf530ccfffbfb80f34164226bc8b16865e5eeeb to cd655b18ba9fb5f8fab0cfaf083b29b0cffaa4c6

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

aa0b75dpermute_indices : Example add and more precise INPUT description
cd655b1Merge branch 'public/manifolds/add_tensor_with_indices-28787' of git://trac.sagemath.org/sage into t/28787/public/manifolds/add_tensor_with_indices-28787

comment:43 Changed 5 months ago by embray

  • Milestone changed from sage-9.0 to sage-9.1

Ticket retargeted after milestone closed

comment:44 Changed 5 months ago by egourgoulhon

May I ask about the status of this ticket? Is it again ready for review?

Last edited 5 months ago by egourgoulhon (previous) (diff)

comment:45 Changed 5 months ago by gh-LBrunswic

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 5 months ago by git

  • Commit changed from cd655b18ba9fb5f8fab0cfaf083b29b0cffaa4c6 to 4340fc877393abc5491f9ceae2ed74c7c4d77b2f

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

4340fc8Merge branch 'develop' into t/28787/public/manifolds/add_tensor_with_indices-28787

comment:47 Changed 5 months ago by gh-LBrunswic

It seems okay to me, I thought I had asked for review after my previous commit.

comment:48 Changed 5 months ago by gh-LBrunswic

  • Status changed from needs_work to needs_review

comment:49 Changed 5 months ago by egourgoulhon

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 5 months ago by tscrim

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 5 months ago by git

  • Commit changed from 4340fc877393abc5491f9ceae2ed74c7c4d77b2f to e5882893d2f9029b3b58e3fb323dbbf04f8fce46

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

e588289Docstring correction

comment:52 Changed 5 months ago by git

  • Commit changed from e5882893d2f9029b3b58e3fb323dbbf04f8fce46 to fde92459be55a20e6cdefc230ea107450495f925

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

fde9245Docstring correction

comment:53 Changed 5 months ago by egourgoulhon

  • 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 5 months ago by gh-LBrunswic

Thanks! I understand the importance of the constraints on the docstrings but it's plainful to get blocked for a whitespace.

comment:55 Changed 5 months ago by vbraun

  • 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 5 months ago by git

  • Commit changed from fde92459be55a20e6cdefc230ea107450495f925 to a1b963050be2f849e23d84cb0d74c6c8fcd45009

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

a1b9630python2 compatibility

comment:57 Changed 5 months ago by gh-LBrunswic

  • Status changed from needs_work to needs_review

comment:58 Changed 5 months ago by tscrim

  • Status changed from needs_review to positive_review

comment:59 Changed 4 months ago by vbraun

  • Branch changed from public/manifolds/add_tensor_with_indices-28787 to a1b963050be2f849e23d84cb0d74c6c8fcd45009
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.