Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding multiple symmetries and multiple contractions to tensors #28784

Closed
LBrunswic mannequin opened this issue Nov 21, 2019 · 29 comments
Closed

Adding multiple symmetries and multiple contractions to tensors #28784

LBrunswic mannequin opened this issue Nov 21, 2019 · 29 comments

Comments

@LBrunswic
Copy link
Mannequin

LBrunswic mannequin commented Nov 21, 2019

As of sage-9.0.beta6, applying multiple contractions or multiple symmetries to tensors in indices notation raises a NotImplementedError.

This tickets aims at removing this error by implementing the adequate behavior as well as adding a convention check on the index notation.

The index notation should allow :

  • Multiple contraction

  • Multiple symmetries

  • indices denoted by a non-accentuated latin caracter {a,...,z,A,...,Z} and wild card "."

  • covariant indices first notation as well as contravariant indices first

  • Latex notations '{' and '}'

  • if indices do not begin by ^ nor _ then contravariant indices first is assumed

The index notation should not allow :

  • Repeated indices of the same type

  • indices denoted by any other caracter

  • nested symmetries

  • unbalanced parentheses/brackets

NB : Usual index notations allows greek indices but their implementation seems more difficult and is not the goal of the ticket.

CC: @egourgoulhon

Component: linear algebra

Keywords: tensor, contraction, symmetries

Author: Léo Brunswic

Branch/Commit: 3013354

Reviewer: Eric Gourgoulhon

Issue created by migration from https://trac.sagemath.org/ticket/28784

@LBrunswic LBrunswic mannequin added this to the sage-9.0 milestone Nov 21, 2019
@LBrunswic LBrunswic mannequin added the p: major / 3 label Nov 21, 2019
@LBrunswic
Copy link
Mannequin Author

LBrunswic mannequin commented Nov 21, 2019

Changed keywords from none to tensor, contraction, symmetries

@LBrunswic

This comment has been minimized.

@LBrunswic
Copy link
Mannequin Author

LBrunswic mannequin commented Nov 21, 2019

Author: Léo Brunswic

@LBrunswic LBrunswic mannequin added t: enhancement labels Nov 21, 2019
@LBrunswic
Copy link
Mannequin Author

LBrunswic mannequin commented Nov 21, 2019

@LBrunswic
Copy link
Mannequin Author

LBrunswic mannequin commented Nov 21, 2019

Commit: 1d1e4ea

@LBrunswic LBrunswic mannequin self-assigned this Nov 21, 2019
@LBrunswic

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 21, 2019

Changed commit from 1d1e4ea to 59c86e3

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 21, 2019

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

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

@LBrunswic LBrunswic mannequin added the s: needs review label Nov 21, 2019
@egourgoulhon
Copy link
Member

comment:7

Thanks for this useful enhancement!

The patchbot reports some failures:

sage -t --long src/sage/manifolds/differentiable/tensorfield.py  # 2 doctests failed
sage -t --long src/sage/tensor/modules/free_module_tensor.py  # 19 doctests failed
sage -t --long src/sage/tensor/modules/tensor_with_indices.py  # 39 doctests failed

Besides, could you add a few doctests to illustrate the new functionalities?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 22, 2019

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 22, 2019

Changed commit from 59c86e3 to 4b9a565

@LBrunswic
Copy link
Mannequin Author

LBrunswic mannequin commented Nov 22, 2019

comment:9

@egourgoulhon
I needed it :)

I'm sorry I forgot to do a doctest :$...I corrected the features that are tested and added new doctest.

@LBrunswic

This comment has been minimized.

@LBrunswic LBrunswic mannequin added s: needs review and removed s: needs work labels Nov 22, 2019
@LBrunswic

This comment has been minimized.

@egourgoulhon
Copy link
Member

New commits:

0823e29Minor corrections for #28784

@egourgoulhon
Copy link
Member

Reviewer: Eric Gourgoulhon

@egourgoulhon
Copy link
Member

Changed commit from 4b9a565 to 0823e29

@egourgoulhon
Copy link
Member

comment:13

Thanks for making all doctest passed.
I gave a more in-depth look at the code and it looks very good.
I've just performed some reviewer tweaks, which I pushed in the new branch. You can see the (minor) changes here.
In particular

  • a doctest block has been corrected by adding a missing ::
  • some PEP 8 enforcement has been performed
  • doctests raising an exception have been rewritten via Traceback... instead of a try block
  • the header # -*- coding: utf-8 -*- has been added to the file tensor_with_indices.py, in order to get rid of the non-ascii error revealed by the patchbot.

Do you agree with the above changes?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 24, 2019

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

3013354Marginal modifications

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 24, 2019

Changed commit from 0823e29 to 3013354

@LBrunswic
Copy link
Mannequin Author

LBrunswic mannequin commented Nov 24, 2019

comment:15

Thanks for the PEP8 enforcement and the correction of the doctest.

Of course, I agree with the changes and merged.
I quickly reviewed them and checked the doctest.

Also, I corrected a typo and clarified two lines.

@LBrunswic
Copy link
Mannequin Author

LBrunswic mannequin commented Nov 24, 2019

comment:16

I would give a positive review but since I modified the code (even marginally) I think I shouldn't do it.

@egourgoulhon
Copy link
Member

comment:17

I've just checked your latest changes. Everything is OK.
Thanks for this work.

@mwageringel
Copy link

comment:18

It would be nice to add this functionality for multiple symmetrizations to the method symmetrize() as well because symmetrization is not necessarily done over neighboring indices which cannot be handled by the string syntax. I assume it is more efficient to do all symmetrizations and antisymmetrizations in a single function call, rather than computing several intermediate results.

@LBrunswic
Copy link
Mannequin Author

LBrunswic mannequin commented Nov 27, 2019

comment:19

Replying to @mwageringel:

It would be nice to add this functionality for multiple symmetrizations to the method symmetrize() as well because symmetrization is not necessarily done over neighboring indices which cannot be handled by the string syntax. I assume it is more efficient to do all symmetrizations and antisymmetrizations in a single function call, rather than computing several intermediate results.

Hi!

I completely agree! We should open another ticket or a discussion of devel to discuss this (I would at the very least ask Eric Gourgoulhon for validation).

To begin the discussion, I see other issues with the current implementation and methods.
For instance, it's not possible to define the symmetries of the riemann tensor. This is related to the fact that (anti)symmetrize ony takes a set of indices as argument and then consider the action of the natural action of the symmetric group this set.
It would be appreciable to define the symmetrization via an action of some group on the set indices. Antisymmetrization would need also a 'signature' morphism G-->{1,-1}. More generally, one could consider linear group action symmetries.

@egourgoulhon
Copy link
Member

comment:20

Replying to @LBrunswic:

Replying to @mwageringel:

It would be nice to add this functionality for multiple symmetrizations to the method symmetrize() as well because symmetrization is not necessarily done over neighboring indices which cannot be handled by the string syntax. I assume it is more efficient to do all symmetrizations and antisymmetrizations in a single function call, rather than computing several intermediate results.

Hi!

I completely agree! We should open another ticket or a discussion of devel to discuss this (I would at the very least ask Eric Gourgoulhon for validation).

Yes please open a ticket for this! (and continue the discussion there).

To begin the discussion, I see other issues with the current implementation and methods.
For instance, it's not possible to define the symmetries of the riemann tensor. This is related to the fact that (anti)symmetrize ony takes a set of indices as argument and then consider the action of the natural action of the symmetric group this set.
It would be appreciable to define the symmetrization via an action of some group on the set indices. Antisymmetrization would need also a 'signature' morphism G-->{1,-1}. More generally, one could consider linear group action symmetries.

Indeed, this would be nice.

@mwageringel
Copy link

comment:21

The new ticket is #28813.

@vbraun
Copy link
Member

vbraun commented Nov 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants