Opened 8 months ago
Closed 8 months ago
#28973 closed defect (fixed)
Automorphism Fields: Notation of Inverse
Reported by:  ghDeRhamSource  Owned by:  

Priority:  major  Milestone:  sage9.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 )
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 tangentspace automorphisms a^(1)*b^(1) on the 2dimensional 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 tangentspace automorphisms (a^(1)*b)^(1) on the 2dimensional 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
 Branch set to u/ghDeRhamSource/automorphism_inverse_notation
comment:2 Changed 8 months ago by
 Commit set to 8b9ac9a24a29930ae84cac2e575ce821171bd4bb
 Status changed from new to needs_review
comment:3 Changed 8 months ago by
 Description modified (diff)
comment:4 Changed 8 months ago by
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
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
 Status changed from needs_review to needs_work
comment:7 Changed 8 months ago by
 Description modified (diff)
 Status changed from needs_work to needs_review
comment:8 Changed 8 months ago by
 Commit changed from 8b9ac9a24a29930ae84cac2e575ce821171bd4bb to 15067c152449793de4e1ca567313ed10b06de245
Branch pushed to git repo; I updated commit sha1. New commits:
15067c1  Trac #28973: is_atomic adapted and used

comment:9 Changed 8 months ago by
 Commit changed from 15067c152449793de4e1ca567313ed10b06de245 to c7fbb72cab8da51db5c461b0b51b2c686a232d75
Branch pushed to git repo; I updated commit sha1. New commits:
c7fbb72  Trac #28973: Small improvement of is_atomic and Python3 compatibility in doctest

comment:10 Changed 8 months ago by
 Commit changed from c7fbb72cab8da51db5c461b0b51b2c686a232d75 to 69ee5289141605e9aa6a1c3683d76b59efa50a7d
Branch pushed to git repo; I updated commit sha1. New commits:
69ee528  Trac #28973: No raw string needed

comment:11 Changed 8 months ago by
 Status changed from needs_review to needs_work
comment:12 Changed 8 months ago by
 Commit changed from 69ee5289141605e9aa6a1c3683d76b59efa50a7d to 48d84fe66ffa84b4976faf6141813aa27f8aaf80
Branch pushed to git repo; I updated commit sha1. New commits:
48d84fe  Trac #28973: Automorphism fields adapted to new code

comment:13 Changed 8 months ago by
 Status changed from needs_work to needs_review
comment:14 Changed 8 months ago by
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 nonempty lines
Could you revert this change?
comment:15 Changed 8 months ago by
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
 Commit changed from 48d84fe66ffa84b4976faf6141813aa27f8aaf80 to db09dbc682b259e619d0019ce7c5ca56ccb01916
comment:17 followup: ↓ 18 Changed 8 months ago by
Like this?
comment:18 in reply to: ↑ 17 Changed 8 months ago by
Replying to ghDeRhamSource:
Like this?
As you can see from the latest patchbot report, <>
still causes some trouble:
Python3 incompatible code inserted on 0 nonempty 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 nonempty 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
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
 Commit changed from db09dbc682b259e619d0019ce7c5ca56ccb01916 to 840304928e17abefa37418de9d702090850e06fd
Branch pushed to git repo; I updated commit sha1. New commits:
8403049  Trac #28973: Doctest improved

comment:21 Changed 8 months ago by
Perhaps this is a good example because the separator consists of more than one letter.
comment:22 Changed 8 months ago by
 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
 Description modified (diff)
comment:24 Changed 8 months ago by
 Branch changed from u/ghDeRhamSource/automorphism_inverse_notation to 840304928e17abefa37418de9d702090850e06fd
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Trac #28973: Brackets around element before inverse