Opened 3 years ago
Closed 3 years ago
#28562 closed enhancement (fixed)
Tensor Fields: Better Zero Treatment
Reported by:  Michael Jung  Owned by:  

Priority:  major  Milestone:  sage9.0 
Component:  geometry  Keywords:  manifolds, tensor fields, scalar fields, mixed forms 
Cc:  Travis Scrimshaw, Eric Gourgoulhon  Merged in:  
Authors:  Michael Jung  Reviewers:  Eric Gourgoulhon, Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  f2928f8 (Commits, GitHub, GitLab)  Commit:  f2928f882a5a8eb345dd11c308e895d615f9de9d 
Dependencies:  Stopgaps: 
Description (last modified by )
The zero element is always a special element. Therefore it should be treated as such. It should shorten computations and certainly be immutable. This ticket is devoted to that topic. (Similarly for the one element in the scalar field and mixed form algebra).
This ticket is part of the metaticket #28519.
Features
 an assertion error arises when altering the fixed elements zero or one
 a new attribute
_is_zero
is added to tensor fields and mixed form (similar to scalar fields)  computations with involved zero or one are shortened by using a simple check
 due to immutability of algebra elements, no copies are returned anymore for scalar field operations with zero or one
_is_zero
attribute is applied for copies
Change History (43)
comment:1 Changed 3 years ago by
Branch:  → u/ghDeRhamSource/better_zero_treatment 

comment:2 Changed 3 years ago by
Commit:  → 9f91503dce5cbe14f52a6a77a34ae5a18f722f7d 

Description:  modified (diff) 
comment:3 Changed 3 years ago by
Commit:  9f91503dce5cbe14f52a6a77a34ae5a18f722f7d → 5094a3e802034be5916879a0225e5db2ccf9f0f8 

Branch pushed to git repo; I updated commit sha1. New commits:
5094a3e  Return no copies for scalar field operations

comment:4 Changed 3 years ago by
Description:  modified (diff) 

comment:5 Changed 3 years ago by
Description:  modified (diff) 

comment:6 Changed 3 years ago by
Commit:  5094a3e802034be5916879a0225e5db2ccf9f0f8 → 94cf06ab1ab5d87fbc8efc73d2688ea1c8c91267 

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

892050e  FreeModuleLinearGroup: one treatment

1b96587  Zero treatment for FreeModules

b9c8287  AutomorphismField: zero treatment + MixedForm: set_restriction and add_comp_by_continuation

94cf06a  Mixed forms adapted to zero treatment + zero treatment for scalar fields

comment:7 Changed 3 years ago by
Component:  PLEASE CHANGE → geometry 

Status:  new → needs_review 
comment:8 Changed 3 years ago by
In MixedForm
, the zero (and one element for scalar fields) is treated separately. The code in there is not very beautiful, but it works for now. However, I plan to reorganize the code of mixed forms anyway (see #28519).
comment:9 Changed 3 years ago by
Also, I removed the _zero_element
attribute in the free modules and replaced it by a cached zero()
method. This is a better solution, I guess.
comment:10 Changed 3 years ago by
Commit:  94cf06ab1ab5d87fbc8efc73d2688ea1c8c91267 → d949a13db29334e193ec9ff05fd7fe53deb65422 

Branch pushed to git repo; I updated commit sha1. New commits:
d949a13  Minor code reduction

comment:11 Changed 3 years ago by
Commit:  d949a13db29334e193ec9ff05fd7fe53deb65422 → 5d30100a5f1e4f809867dc5b564000cd2eddc9db 

Branch pushed to git repo; I updated commit sha1. New commits:
5d30100  More code reduction

comment:12 Changed 3 years ago by
The code snippet (see commits 5d30100 and d949a13) was quite messy. Well, it still is. At least it is readable and works for now. It gets a little bit better after merging (see #28519).
I will think about a better solution. But I guess, this requires a complete reorganization of the mixed form code.
comment:13 followups: 15 17 Changed 3 years ago by
Status:  needs_review → needs_info 

After a bit of thinking, I came to the conclusion that the code snippet should be obsolete. (As a mathematician, I feel the urge to solve or fix a problem/error as soon as it occurs. Here: Doctest errors.) The thing is, due to our little discussion in #28519, the doctest must be flawed since mixed forms defined via preconstructed forms get altered.
So, I 'm going to delete this snippet again and modify the affected doctests.
Do you agree with my conclusion and procedure?
A further doctest change is devoted to #28578.
comment:14 Changed 3 years ago by
Description:  modified (diff) 

comment:15 followup: 16 Changed 3 years ago by
Replying to ghDeRhamSource:
After a bit of thinking, I came to the conclusion that the code snippet should be obsolete.
Which code snippet are you talking about? All the code in this ticket branch?
comment:16 Changed 3 years ago by
Replying to egourgoulhon:
Replying to ghDeRhamSource:
After a bit of thinking, I came to the conclusion that the code snippet should be obsolete.
Which code snippet are you talking about? All the code in this ticket branch?
No, only the last two/three commits. Namely the changes in set_restriction
of mixed_form.py
.
comment:17 Changed 3 years ago by
Replying to ghDeRhamSource:
After a bit of thinking, I came to the conclusion that the code snippet should be obsolete. (As a mathematician, I feel the urge to solve or fix a problem/error as soon as it occurs. Here: Doctest errors.) The thing is, due to our little discussion in #28519, the doctest must be flawed since mixed forms defined via preconstructed forms get altered.
So, I 'm going to delete this snippet again and modify the affected doctests.
Do you agree with my conclusion and procedure?
I am not sure to understand what you have in mind; I would say please go on and implement what you think is the best solution.
comment:18 Changed 3 years ago by
sage: M = Manifold(2, 'M') # the 2dimensional sphere S^2 sage: U = M.open_subset('U') # complement of the North pole sage: c_xy.<x,y> = U.chart() # stereographic coordinates from the North pole sage: V = M.open_subset('V') # complement of the South pole sage: c_uv.<u,v> = V.chart() # stereographic coordinates from the South pole sage: M.declare_union(U,V) # S^2 is the union of U and V sage: xy_to_uv = c_xy.transition_map(c_uv, (x/(x^2+y^2), y/(x^2+y^2)), ....: intersection_name='W', restrictions1= x^2+y^2!=0, ....: restrictions2= u^2+v^2!=0) sage: uv_to_xy = xy_to_uv.inverse() sage: e_xy = c_xy.frame(); e_uv = c_uv.frame() sage: f = M.scalar_field(x, name='f') sage: omega = M.diff_form(1, name='omega') sage: omega[e_xy,0] = x sage: F = M.mixed_form(name='F', comp=[f, omega, 0]) sage: F.add_comp_by_continuation(e_xy, U.intersection(V), c_xy) sage: F.add_comp_by_continuation(e_uv, U.intersection(V), c_uv)
This is an example of how I used add_comp_by_continuation
in the doctest. Obviously, this is a bad example. To ensure the "immutability" of mixed forms, the comp
attribute should be used for already fully defined differential forms only. Whenever a incremental definition is explicitly wanted, F[1][e_xy,0] = x
or F[1] = M.one_form({e_xy: [x,0]}, name='omega')
should be used instead, and then add_comp_by_continuation
is reasonable to use.
At least, that is my suggestion of how to use mixed forms.
Travis? Would you agree?
comment:19 Changed 3 years ago by
I am not sure I completely understand the question, or the subtleties in the use case. So I don't think I can make a reasonable comment here.
comment:20 Changed 3 years ago by
Mh. Sorry for that. I try again: The thing in the example above is that the zero element gets manipulated via add_comp_by_continuation
, or set_restriction
respectively. This indicates, the whole example (even if there is no zero element involved) is a bad use of this methods.
Better would be:
sage: M = Manifold(2, 'M') # the 2dimensional sphere S^2 sage: U = M.open_subset('U') # complement of the North pole sage: c_xy.<x,y> = U.chart() # stereographic coordinates from the North pole sage: V = M.open_subset('V') # complement of the South pole sage: c_uv.<u,v> = V.chart() # stereographic coordinates from the South pole sage: M.declare_union(U,V) # S^2 is the union of U and V sage: xy_to_uv = c_xy.transition_map(c_uv, (x/(x^2+y^2), y/(x^2+y^2)), ....: intersection_name='W', restrictions1= x^2+y^2!=0, ....: restrictions2= u^2+v^2!=0) sage: uv_to_xy = xy_to_uv.inverse() sage: e_xy = c_xy.frame(); e_uv = c_uv.frame() sage: f = M.scalar_field(x, name='f') sage: f.add_expr_by_continuation(c_uv, U.intersection(V)) sage: omega = M.diff_form(1, name='omega') sage: omega[e_xy,0] = x sage: omega.add_comp_by_continuation(e_uv, U.intersection(V), c_uv) sage: F = M.mixed_form(name='F', comp=[f, omega, 0])
Yet, I noticed that add_comp_by_continuation
seems not being convenient at all. It runs into problems as soon as there is a homogeneous component defined which should not be altered or has no tensor component in the corresponding intersection domain. Therefore, if one single homogeneous component shall be zero, and all other components shall be continued, which is a reasonable use, this methods fails to fulfill its purpose.
Sorry, I'm not that much into that kind of thinking, especially the immutability of sage objects, and may confuse all around me including myself. :D
comment:21 Changed 3 years ago by
Commit:  5d30100a5f1e4f809867dc5b564000cd2eddc9db → 3f1169874af7db4200bb91544854bb9f90afc9fc 

Branch pushed to git repo; I updated commit sha1. New commits:
3f11698  Continuations of zero and one untouched

comment:23 Changed 3 years ago by
I guess it depends on how you define zero. Although if I think of this like a graded module, 0
exists in all graded parts simultaneously, so by analogy, the add_comp_by_continuation
does work correctly. Although as a computer implementation that requires some more precision, this might not be feasible.
comment:24 Changed 3 years ago by
Status:  needs_review → needs_work 

Yes, you're absolutely right. So, seeing things more from the mathematical point of view is always the way to go, right?
The problem here is that add_comp_by_continuation
invokes the add_comp
method for tensor fields which raises an error when it is used by the zero element. To make it usable in this case, a separate treatment of zero must be implemented.
comment:25 Changed 3 years ago by
Branch:  u/ghDeRhamSource/better_zero_treatment → u/ghDeRhamSource/better_zero_treatment_alternative 

Commit:  3f1169874af7db4200bb91544854bb9f90afc9fc 
Status:  needs_work → needs_review 
I started the whole thing again from scratch since add_comp_by_continuation
did not work properly in a mathematical manner.
Moreover, I (hopefully) improved the preexisting code.
In the commits, there are two alternatives presented: One using a flag check_elements
and one using a separate method without security check. I'd prefer the second solution. It is completely hidden for the user and may become convenient when more checks are established in the future.
comment:26 Changed 3 years ago by
Commit:  → 286553516abfc2895b4136fda99ce7fff653ff4c 

Branch pushed to git repo; I updated commit sha1. New commits:
9f91503  '_is_zero' attribute added and modified

5094a3e  Return no copies for scalar field operations

e016b21  Zero and one treatment for free tensor modules

371a255  Zero treatment for wedge product of free alt forms

1b1ee7f  Zero/One check added

5bcc7af  Alternative solution via unsafe method

2865535  Obsolete '**kwargs' removed

comment:27 Changed 3 years ago by
What I wanted to say with my previous message: I am not a professional programmer, however I had quite a few difficulties in modifying the code and perform a proper debugging. There are some code redundancies and hidden dependencies which could be susceptible for further modifications (like: a modification of add_comp
in tensorfield.py
caused severe troubles in automorphismfield.py
). Perhaps, some code delegations and/or reusabilities are useful in the farther future?
comment:28 Changed 3 years ago by
Commit:  286553516abfc2895b4136fda99ce7fff653ff4c → c104fc844d8cd50446944d0cd1b177c369366653 

Branch pushed to git repo; I updated commit sha1. New commits:
c104fc8  Scalar field zero treatment

comment:29 Changed 3 years ago by
Commit:  c104fc844d8cd50446944d0cd1b177c369366653 → efd8e0390aa99ee7d8c667e3120df43ee624e59a 

Branch pushed to git repo; I updated commit sha1. New commits:
efd8e03  Mixed form zero treatment

comment:30 Changed 3 years ago by
I just gave a quick look at the latest version. Looks good. I'll have a deeper look tomorrow.
comment:31 Changed 3 years ago by
Commit:  efd8e0390aa99ee7d8c667e3120df43ee624e59a → a91dba0f83f5bd003f339a834735ea99cd5a510d 

comment:32 Changed 3 years ago by
Branch:  u/ghDeRhamSource/better_zero_treatment_alternative → public/manifolds/better_zero_treatment 

Commit:  a91dba0f83f5bd003f339a834735ea99cd5a510d → 2783253621fa561b0b465ec8fccc30a799b72d88 
I've finally gone through the ticket. The impression stated in comment:30 is confirmed: this is a nice improvement of the code! Thank you. I have made minor modifications, which have been pushed in the above branch. In particular
 the pyflake error reported by the patchbot is corrected
t(0)
has been changed tot.zero()
andgl(1)
togl.one()
(to skip one function call) some
add/set_comp(basis)
have been changed toadd/set_comp(basis=basis)
 the treatment of zero has been added in
FreeModuleTensor._sub_()
Do you agree with these changes?
New commits:
2783253  Minor changes in the zero and one treatment of tensor fields

comment:34 followup: 39 Changed 3 years ago by
I just have two trivial changes:
The components w.r.t. basis e have been kept:: +The components with respect to basis ``e`` have been kept::
and in automorphismfield.py
:
def add_comp(self, basis=None): r""" Return the components of ``self`` w.r.t. a given module basis for assignment, keeping the components w.r.t. other bases. To delete the components w.r.t. other bases, use the method :meth:`set_comp` instead.
remove the blank line after r"""
and same thing of spelling out w.r.t.
(although I suspect this is done elsewhere, I think it is better to avoid abbreviations like this as this is a more "formal" document, but I don't care too much).
comment:35 Changed 3 years ago by
On my side, one last remark: the names of the methods _add_comp_unsafe()
and _set_comp_unsafe()
give the impression that they are really dangerous methods that should be avoided, while they are key methods, doing most of the work of add_comp()
and set_comp()
.
Since they are *private* methods, I don't think it is necessary to insist too much on their "unsafe" aspect, which is truly unsafe only when invoked on the special elements zero and one. Wouldn't something like _add_comp_nocheck()
and _set_comp_nocheck()
be better names? Travis, do you have an opinion on that?
comment:36 followup: 37 Changed 3 years ago by
This naming convention matches what is done for vectors (and maybe matrices?) in Sage. Although in that case, if you mess with things in the wrong way, you will crash Sage. However, given the nomenclature of other methods, I think *_unsafe
is the way to go.
comment:37 Changed 3 years ago by
Replying to tscrim:
This naming convention matches what is done for vectors (and maybe matrices?) in Sage.
Ah yes! (grep r "_unsafe"
in src/sage
returns 872 entries; I was not aware of that). So OK for *_unsafe
then...
comment:38 Changed 3 years ago by
Commit:  2783253621fa561b0b465ec8fccc30a799b72d88 → f2928f882a5a8eb345dd11c308e895d615f9de9d 

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

comment:39 followup: 40 Changed 3 years ago by
Replying to tscrim:
I just have two trivial changes:
The components w.r.t. basis e have been kept:: +The components with respect to basis ``e`` have been kept::
Since this issue is not just devoted to this ticket, I vote for a new ticket on that.
and in
automorphismfield.py
:def add_comp(self, basis=None): r""" Return the components of ``self`` w.r.t. a given module basis for assignment, keeping the components w.r.t. other bases. To delete the components w.r.t. other bases, use the method :meth:`set_comp` instead.remove the blank line after
r"""
and same thing of spelling outw.r.t.
(although I suspect this is done elsewhere, I think it is better to avoid abbreviations like this as this is a more "formal" document, but I don't care too much).
Done.
comment:40 Changed 3 years ago by
Replying to ghDeRhamSource:
Replying to tscrim:
I just have two trivial changes:
The components w.r.t. basis e have been kept:: +The components with respect to basis ``e`` have been kept::Since this issue is not just devoted to this ticket, I vote for a new ticket on that.
Indeed. I am afraid I am responsible for most of these w.r.t.
;)
comment:41 Changed 3 years ago by
Status:  needs_review → positive_review 

Thanks for your work on this!
comment:42 Changed 3 years ago by
Reviewers:  → Eric Gourgoulhon, Travis Scrimshaw 

comment:43 Changed 3 years ago by
Branch:  public/manifolds/better_zero_treatment → f2928f882a5a8eb345dd11c308e895d615f9de9d 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
'_is_zero' attribute added and modified