Opened 8 months ago

Closed 8 months ago

#28973 closed defect (fixed)

Automorphism Fields: Notation of Inverse

Reported by: gh-DeRhamSource Owned by:
Priority: major Milestone: sage-9.1
Component: geometry Keywords: automorphisms
Cc: egourgoulhon, tscrim Merged in:
Authors: Michael Jung Reviewers: Eric Gourgoulhon
Report Upstream: N/A Work issues:
Branch: 8403049 (Commits) Commit: 840304928e17abefa37418de9d702090850e06fd
Dependencies: Stopgaps:

Description (last modified by gh-DeRhamSource)

At this stage we have the following problem with the notation of inverses:

sage: M = Manifold(2, 'M')
sage: X.<x,y> = M.chart()
sage: aib = M.automorphism_field([[0, 2], [-1, 0]], name='a^(-1)*b')
sage: c = aib.inverse(); c
Field of tangent-space automorphisms a^(-1)*b^(-1) on the 2-dimensional differentiable manifold M

That is definitely misleading. In this ticket it gets fixed by adding brackets around the element before taking its inverse if the name could cause any confusion:

sage: c = aib.inverse(); c
Field of tangent-space automorphisms (a^(-1)*b)^(-1) on the 2-dimensional differentiable manifold M

To solve this, the ticket provides a modification of is_atomic in sage/tensor/modules/format_utilities.py.

Change History (24)

comment:1 Changed 8 months ago by gh-DeRhamSource

  • Branch set to u/gh-DeRhamSource/automorphism_inverse_notation

comment:2 Changed 8 months ago by gh-DeRhamSource

  • Commit set to 8b9ac9a24a29930ae84cac2e575ce821171bd4bb
  • Status changed from new to needs_review

New commits:

8b9ac9aTrac #28973: Brackets around element before inverse

comment:3 Changed 8 months ago by gh-DeRhamSource

  • Description modified (diff)

comment:4 Changed 8 months ago by egourgoulhon

Thanks for taking care of this! However, for simple names like a, having (a)^(-1) for the inverse is not desirable. One should maintain a^(-1) in this case. To cover all cases, you should use something like is_atomic() (defined in src/sage/tensor/modules/format_utilities.py); see the handling of names in FreeModuleAltForm.wedge(). More precisely, you should define, in format_utilities.py, a new function similar to is_atomic and adapted to the inverse naming.

comment:5 Changed 8 months ago by gh-DeRhamSource

You're right. That's definitely preferrable.

This was the fastest solution I had in mind so far. I see what I can do.

comment:6 Changed 8 months ago by gh-DeRhamSource

  • Status changed from needs_review to needs_work

comment:7 Changed 8 months ago by gh-DeRhamSource

  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:8 Changed 8 months ago by git

  • Commit changed from 8b9ac9a24a29930ae84cac2e575ce821171bd4bb to 15067c152449793de4e1ca567313ed10b06de245

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

15067c1Trac #28973: is_atomic adapted and used

comment:9 Changed 8 months ago by git

  • Commit changed from 15067c152449793de4e1ca567313ed10b06de245 to c7fbb72cab8da51db5c461b0b51b2c686a232d75

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

c7fbb72Trac #28973: Small improvement of is_atomic and Python3 compatibility in doctest

comment:10 Changed 8 months ago by git

  • Commit changed from c7fbb72cab8da51db5c461b0b51b2c686a232d75 to 69ee5289141605e9aa6a1c3683d76b59efa50a7d

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

69ee528Trac #28973: No raw string needed

comment:11 Changed 8 months ago by gh-DeRhamSource

  • Status changed from needs_review to needs_work

comment:12 Changed 8 months ago by git

  • Commit changed from 69ee5289141605e9aa6a1c3683d76b59efa50a7d to 48d84fe66ffa84b4976faf6141813aa27f8aaf80

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

48d84feTrac #28973: Automorphism fields adapted to new code

comment:13 Changed 8 months ago by gh-DeRhamSource

  • Status changed from needs_work to needs_review

comment:14 Changed 8 months ago by egourgoulhon

Thanks for the useful enhancement of the function is_atomic().

The patchbot complains about the change introduced in the commit of comment:10

+inside file:  b/src/sage/tensor/modules/format_utilities.py
+@@ -52,16 +55,31 @@ def is_atomic(expression):
++        sage: is_atomic("a<>b", sep=['<>'])
++        sage: is_atomic("(a<>b)", sep=['<>'])
+Python3 incompatible code inserted on 2 non-empty lines

Could you revert this change?

comment:15 Changed 8 months ago by egourgoulhon

There is also a doctest error reported by one of the patchbots:

File "src/sage/manifolds/differentiable/automorphismfield.py", line 951, in sage.manifolds.differentiable.automorphismfield.AutomorphismFieldParal
Failed example:
    latex(inv)
Expected:
    R
Got:
    R^{-1}
**********************************************************************
1 item had failures:
   1 of  12 in sage.manifolds.differentiable.automorphismfield.AutomorphismFieldParal

comment:16 Changed 8 months ago by git

  • Commit changed from 48d84fe66ffa84b4976faf6141813aa27f8aaf80 to db09dbc682b259e619d0019ce7c5ca56ccb01916

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

dce291fTrac #28973: Python3 compatibility and doctest
db09dbcTrac #28973: Merge branch 'develop' into automorphism_inverse_notation

comment:17 follow-up: Changed 8 months ago by gh-DeRhamSource

Like this?

comment:18 in reply to: ↑ 17 Changed 8 months ago by egourgoulhon

Replying to gh-DeRhamSource:

Like this?

As you can see from the latest patchbot report, <> still causes some trouble:

-Python3 incompatible code inserted on 0 non-empty lines
+inside file:  b/src/sage/tensor/modules/format_utilities.py
+@@ -52,16 +55,31 @@ def is_atomic(expression):
++        sage: is_atomic(r"a<>b", sep=[r'<>'])
++        sage: is_atomic(r"(a<>b)", sep=[r'<>'])
+Python3 incompatible code inserted on 2 non-empty lines

Apparently, the operator <> has been removed in Python 3: it does not appear in the list https://docs.python.org/3/library/operator.html. So please remove this doctest.

comment:19 Changed 8 months ago by gh-DeRhamSource

I see. Well, this is on the symbolic level, so no actual Python code is involved here. Even though another operator as example would be certainly more convenient.

comment:20 Changed 8 months ago by git

  • Commit changed from db09dbc682b259e619d0019ce7c5ca56ccb01916 to 840304928e17abefa37418de9d702090850e06fd

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

8403049Trac #28973: Doctest improved

comment:21 Changed 8 months ago by gh-DeRhamSource

Perhaps this is a good example because the separator consists of more than one letter.

Last edited 8 months ago by gh-DeRhamSource (previous) (diff)

comment:22 Changed 8 months ago by egourgoulhon

  • Reviewers set to Eric Gourgoulhon
  • Status changed from needs_review to positive_review

Thanks for the change. The patchbot is now full green ==> good to go.

comment:23 Changed 8 months ago by gh-DeRhamSource

  • Description modified (diff)

comment:24 Changed 8 months ago by vbraun

  • Branch changed from u/gh-DeRhamSource/automorphism_inverse_notation to 840304928e17abefa37418de9d702090850e06fd
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.