Opened 2 years ago
Closed 23 months ago
#28564 closed enhancement (fixed)
Tensor Fields and Sections: Naming Consistencies
Reported by:  ghDeRhamSource  Owned by:  

Priority:  major  Milestone:  sage9.0 
Component:  geometry  Keywords:  manifolds, tensor fields, scalar fields 
Cc:  tscrim, egourgoulhon  Merged in:  
Authors:  Michael Jung  Reviewers:  Eric Gourgoulhon, Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  34f2fa1 (Commits, GitHub, GitLab)  Commit:  34f2fa1a4014bf5e3d7c5f54d83aa93216d6efef 
Dependencies:  Stopgaps: 
Description (last modified by )
This ticket is part of the metaticket #28519 and consists of:
 copies apply no name from the original in any case
copy
method with individual naming (right) multiplication with a scalar field
f
yields to the namef*a
and the LaTeX namef \cdot a
for any tensor/section (or scalar) fielda
Change History (49)
comment:1 Changed 2 years ago by
 Cc tscrim egourgoulhon added
 Component changed from PLEASE CHANGE to geometry
 Description modified (diff)
 Keywords manifolds tensor fields scalar fields added
 Type changed from PLEASE CHANGE to enhancement
comment:2 Changed 2 years ago by
 Branch set to u/ghDeRhamSource/tensor_fields__consistent_naming
comment:3 Changed 2 years ago by
 Commit set to 99c392c7f0bec8145544a2048d00c05eb73b0e79
comment:4 Changed 2 years ago by
 Reviewers set to Eric Gourgoulhon, Travis Scrimshaw
 Status changed from new to needs_review
comment:5 Changed 2 years ago by
 Commit changed from 99c392c7f0bec8145544a2048d00c05eb73b0e79 to b3e04682ae022ce8417f99f7d9145fb8a2782676
Branch pushed to git repo; I updated commit sha1. New commits:
b3e0468  MixedForm copies apply names

comment:6 followup: ↓ 7 Changed 2 years ago by
 Status changed from needs_review to needs_info
Actually, what do you prefer? Should copies apply the names from the original or not?
I think, at least it must be consistent...
comment:7 in reply to: ↑ 6 ; followup: ↓ 8 Changed 2 years ago by
Replying to ghDeRhamSource:
Actually, what do you prefer? Should copies apply the names from the original or not?
I don't have a strong opinion on that. Travis?
comment:8 in reply to: ↑ 7 ; followup: ↓ 9 Changed 2 years ago by
Replying to egourgoulhon:
Replying to ghDeRhamSource:
Actually, what do you prefer? Should copies apply the names from the original or not?
I don't have a strong opinion on that. Travis?
I might say no just because copies are meant to be modified, so they should have different names. However, like Eric, this is not a strong opinion. Actually, is there a mechanism to change the name?
comment:9 in reply to: ↑ 8 ; followup: ↓ 10 Changed 2 years ago by
Replying to tscrim:
Replying to egourgoulhon:
I might say no just because copies are meant to be modified, so they should have different names. However, like Eric, this is not a strong opinion.
Thanks for your answer.
Actually, is there a mechanism to change the name?
Yes there is the method set_name()
. One could also pass the (possible) new name as an argument to copy()
. There are a few examples (2 actually) of copy()
taking some arguments in Sage library.
comment:10 in reply to: ↑ 9 Changed 2 years ago by
Replying to egourgoulhon:
Replying to tscrim:
Actually, is there a mechanism to change the name?
Yes there is the method
set_name()
. One could also pass the (possible) new name as an argument tocopy()
. There are a few examples (2 actually) ofcopy()
taking some arguments in Sage library.
Hmm...in that case I probably would say to not copy the name (with a .. WARNING::
(perhaps .. NOTE::
) block about that) and have copy()
take an optional argument for the (latex) name.
comment:11 Changed 2 years ago by
 Commit changed from b3e04682ae022ce8417f99f7d9145fb8a2782676 to f55b881b1126c44b11f5cb0b6b5157721cd75983
comment:12 followup: ↓ 13 Changed 2 years ago by
That's strange. I cannot observe merge conflicts. What's the matter?
comment:13 in reply to: ↑ 12 Changed 2 years ago by
Replying to ghDeRhamSource:
That's strange. I cannot observe merge conflicts. What's the matter?
It's because your develop branch is still 9.0.beta2 (this is what I see from the commit e4470b5). You should update to 9.0.beta3.
comment:14 Changed 2 years ago by
 Commit changed from f55b881b1126c44b11f5cb0b6b5157721cd75983 to f85de87a572fb6dc088e47518cd0acf163cbff41
Branch pushed to git repo; I updated commit sha1. New commits:
f85de87  Merge branch 'develop' into t/28564/tensor_fields__consistent_naming

comment:16 Changed 2 years ago by
 Status changed from needs_review to needs_work
As revealed by the patchbot, there is one failed doctest:
sage t long src/sage/manifolds/differentiable/tensorfield.py ********************************************************************** File "src/sage/manifolds/differentiable/tensorfield.py", line 260, in sage.manifolds.differentiable.tensorfield.TensorField Failed example: s = f*t; s # long time Expected: Tensor field of type (0,2) on the 2dimensional differentiable manifold S^2 Got: Tensor field f*t of type (0,2) on the 2dimensional differentiable manifold S^2
comment:17 followup: ↓ 20 Changed 2 years ago by
Another point: the introduction of the treatment of the names in FreeModuleTensor._rmul_()
is a valuable enhancement, thanks. However, I wonder about the \cdot
in the LaTeX output: _rmul_()
deals with the left multiplication of a module element by an element of the base ring and in most textbooks, this is denoted without any multiplication symbol, i.e. as \lambda x
, where \lambda
is the ring element and x
is the module element. This is certainly true in the two textbooks that we quote regarding free modules:
 R. Godement : *Algebra* [God1968]_
 S. Lang : *Algebra* [Lan2002]_
Granted: Wikipedia article uses the dot...
comment:18 Changed 2 years ago by
 Commit changed from f85de87a572fb6dc088e47518cd0acf163cbff41 to eb92e6aa9a646360ea7a33c2bc27055c7386dc08
Branch pushed to git repo; I updated commit sha1. New commits:
eb92e6a  Doctest fixed

comment:19 Changed 2 years ago by
Mhm. I partially agree with you. In practice, the notation is certainly inconvenient and everything is clear by choosing correct letters, but in this case I guess everything has to be rigorous. The \cdot
makes unambiguously clear that an operation was performed on both elements.
The doctest is fixed. This is indeed a doctest error, but strangely it didn't occur to me in the first place.
comment:20 in reply to: ↑ 17 ; followup: ↓ 21 Changed 2 years ago by
Replying to egourgoulhon:
Granted: Wikipedia article uses the dot...
On the other hand, the dotfree notation is used in Wikipedia article 'Vector space'.
Travis, do you have an opinion on that? In particular, do we have other modules in Sage with named elements? If so, what is the adopted convention? (I think we should be consistent there).
comment:21 in reply to: ↑ 20 ; followup: ↓ 22 Changed 2 years ago by
Replying to egourgoulhon:
Replying to egourgoulhon:
Granted: Wikipedia article uses the dot...
On the other hand, the dotfree notation is used in Wikipedia article 'Vector space'.
Travis, do you have an opinion on that? In particular, do we have other modules in Sage with named elements? If so, what is the adopted convention? (I think we should be consistent there).
I don't know of any other place in Sage where we do something like this. Although at \cdot
is much better than a period for an action IMO ^^;;
.
comment:22 in reply to: ↑ 21 Changed 2 years ago by
Replying to tscrim:
Replying to egourgoulhon:
Replying to egourgoulhon:
Granted: Wikipedia article uses the dot...
On the other hand, the dotfree notation is used in Wikipedia article 'Vector space'.
Travis, do you have an opinion on that? In particular, do we have other modules in Sage with named elements? If so, what is the adopted convention? (I think we should be consistent there).
I don't know of any other place in Sage where we do something like this. Although at
\cdot
is much better than a period for an action IMO^^;;
.
OK. Let us keep the \cdot
then...
comment:23 Changed 2 years ago by
We are almost done here I think. One very last thing: you should modify the ticket description, to take into account the change in copy()
.
comment:24 Changed 2 years ago by
 Commit changed from eb92e6aa9a646360ea7a33c2bc27055c7386dc08 to 5b2eee2c3ef6f6d7613255b746e08beca7e99e23
Branch pushed to git repo; I updated commit sha1. New commits:
5b2eee2  'copy' with individual naming

comment:25 Changed 2 years ago by
 Description modified (diff)
 Status changed from needs_work to needs_review
 Summary changed from Tensor Fields: Consistent Naming to Tensor Fields and Sections: Naming Consistencies
New commits:
5b2eee2  'copy' with individual naming

comment:27 Changed 2 years ago by
 Description modified (diff)
comment:29 Changed 2 years ago by
 Commit changed from 5b2eee2c3ef6f6d7613255b746e08beca7e99e23 to 38aa7f5c8cdf9d1ac3d79238071e44cf43fe90c7
Branch pushed to git repo; I updated commit sha1. New commits:
38aa7f5  Merge branch 'develop' into t/28564/tensor_fields__consistent_naming

comment:30 followup: ↓ 31 Changed 2 years ago by
 Status changed from needs_work to needs_review
That should be it. Please check.
comment:31 in reply to: ↑ 30 ; followup: ↓ 32 Changed 2 years ago by
Replying to ghDeRhamSource:
That should be it. Please check.
Did you get a merge conflict when merging in beta4? Otherwise, there is a merge conflict with the upcoming beta5.
comment:32 in reply to: ↑ 31 Changed 2 years ago by
Replying to tscrim:
Replying to ghDeRhamSource:
That should be it. Please check.
Did you get a merge conflict when merging in beta4?
Yes.
comment:33 Changed 2 years ago by
 Status changed from needs_review to positive_review
Great, thanks.
comment:34 Changed 2 years ago by
 Status changed from positive_review to needs_work
Merge conflict, possibly #28578... just wait for beta5
comment:35 Changed 2 years ago by
 Commit changed from 38aa7f5c8cdf9d1ac3d79238071e44cf43fe90c7 to 9d39cb5ac1387653256b7fa684465dc816ed4efe
Branch pushed to git repo; I updated commit sha1. New commits:
9d39cb5  Merge branch 'develop' into t/28564/tensor_fields__consistent_naming

comment:36 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:37 Changed 2 years ago by
 Status changed from needs_review to positive_review
comment:38 Changed 2 years ago by
 Status changed from positive_review to needs_work
Doctest failure
sage t long /usr/lib/python3.7/sitepackages/sage/manifolds/differentiable/tensorfield.py ********************************************************************** File "/usr/lib/python3.7/sitepackages/sage/manifolds/differentiable/tensorfield.py", line 269, in sage.manifolds.differentiable.tensorfield.TensorField Failed example: s = f*t.restrict(U); s Expected: Tensor field of type (0,2) on the Open subset U of the 2dimensional differentiable manifold S^2 Got: Tensor field f*t of type (0,2) on the Open subset U of the 2dimensional differentiable manifold S^2
One such doctest was fixed in the same file at line 263 but this one was missed.
comment:39 Changed 2 years ago by
 Commit changed from 9d39cb5ac1387653256b7fa684465dc816ed4efe to 67e5cdc2b5414bd5f1d1accfbd5d7684de21b765
Branch pushed to git repo; I updated commit sha1. New commits:
67e5cdc  Trac #28564: One line doctest error fixed

comment:40 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:41 Changed 2 years ago by
 Status changed from needs_review to positive_review
comment:42 Changed 2 years ago by
 Commit changed from 67e5cdc2b5414bd5f1d1accfbd5d7684de21b765 to 5dd53b08816502d1b1ee00f911bc80f1b426fb00
 Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
5dd53b0  Trac #28564: raw strings for LaTeX code

comment:43 Changed 2 years ago by
Now, everything must be fine, I guess.
comment:44 Changed 2 years ago by
Please give it a positive review, again. Sorry for this trivial mistake in the first place.
comment:45 Changed 2 years ago by
 Status changed from needs_review to positive_review
Thanks for the fix.
comment:46 Changed 2 years ago by
 Commit changed from 5dd53b08816502d1b1ee00f911bc80f1b426fb00 to 34f2fa1a4014bf5e3d7c5f54d83aa93216d6efef
 Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
34f2fa1  Trac #28564: Merge branch 'develop' into t/28564/tensor_fields__consistent_naming

comment:47 Changed 2 years ago by
Please give it a positive review again...
comment:48 Changed 2 years ago by
 Status changed from needs_review to positive_review
The 9.0.beta6 patchbot is all green.
comment:49 Changed 23 months ago by
 Branch changed from u/ghDeRhamSource/tensor_fields__consistent_naming to 34f2fa1a4014bf5e3d7c5f54d83aa93216d6efef
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Copy and multiplication names