Opened 3 years ago
Closed 3 years 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, GitHub, GitLab) | 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 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 3 years ago by
- Branch set to u/gh-DeRhamSource/automorphism_inverse_notation
comment:2 Changed 3 years ago by
- Commit set to 8b9ac9a24a29930ae84cac2e575ce821171bd4bb
- Status changed from new to needs_review
comment:3 Changed 3 years ago by
- Description modified (diff)
comment:4 Changed 3 years 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 3 years 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 3 years ago by
- Status changed from needs_review to needs_work
comment:7 Changed 3 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
comment:8 Changed 3 years 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 3 years 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 3 years 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 3 years ago by
- Status changed from needs_review to needs_work
comment:12 Changed 3 years 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 3 years ago by
- Status changed from needs_work to needs_review
comment:14 Changed 3 years 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 non-empty lines
Could you revert this change?
comment:15 Changed 3 years 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 3 years ago by
- Commit changed from 48d84fe66ffa84b4976faf6141813aa27f8aaf80 to db09dbc682b259e619d0019ce7c5ca56ccb01916
comment:17 follow-up: ↓ 18 Changed 3 years ago by
Like this?
comment:18 in reply to: ↑ 17 Changed 3 years ago by
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 3 years 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 3 years 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 3 years ago by
Perhaps this is a good example because the separator consists of more than one letter.
comment:22 Changed 3 years 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 3 years ago by
- Description modified (diff)
comment:24 Changed 3 years ago by
- Branch changed from u/gh-DeRhamSource/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