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: sage-9.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:

Status badges

Description (last modified by Michael Jung)

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 Michael Jung

Branch: u/gh-DeRhamSource/better_zero_treatment

comment:2 Changed 3 years ago by Michael Jung

Commit: 9f91503dce5cbe14f52a6a77a34ae5a18f722f7d
Description: modified (diff)

New commits:

9f91503'_is_zero' attribute added and modified

comment:3 Changed 3 years ago by git

Commit: 9f91503dce5cbe14f52a6a77a34ae5a18f722f7d5094a3e802034be5916879a0225e5db2ccf9f0f8

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

5094a3eReturn no copies for scalar field operations

comment:4 Changed 3 years ago by Michael Jung

Description: modified (diff)

comment:5 Changed 3 years ago by Michael Jung

Description: modified (diff)

comment:6 Changed 3 years ago by git

Commit: 5094a3e802034be5916879a0225e5db2ccf9f0f894cf06ab1ab5d87fbc8efc73d2688ea1c8c91267

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

ca8dd88Modules modified
892050eFreeModuleLinearGroup: one treatment
1b96587Zero treatment for FreeModules
b9c8287AutomorphismField: zero treatment + MixedForm: set_restriction and add_comp_by_continuation
94cf06aMixed forms adapted to zero treatment + zero treatment for scalar fields

comment:7 Changed 3 years ago by Michael Jung

Component: PLEASE CHANGEgeometry
Status: newneeds_review

comment:8 Changed 3 years ago by Michael Jung

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).

Last edited 3 years ago by Michael Jung (previous) (diff)

comment:9 Changed 3 years ago by Michael Jung

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 git

Commit: 94cf06ab1ab5d87fbc8efc73d2688ea1c8c91267d949a13db29334e193ec9ff05fd7fe53deb65422

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

d949a13Minor code reduction

comment:11 Changed 3 years ago by git

Commit: d949a13db29334e193ec9ff05fd7fe53deb654225d30100a5f1e4f809867dc5b564000cd2eddc9db

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

5d30100More code reduction

comment:12 Changed 3 years ago by Michael Jung

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.

Last edited 3 years ago by Michael Jung (previous) (diff)

comment:13 Changed 3 years ago by Michael Jung

Status: needs_reviewneeds_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.

Last edited 3 years ago by Michael Jung (previous) (diff)

comment:14 Changed 3 years ago by Michael Jung

Description: modified (diff)

comment:15 in reply to:  13 ; Changed 3 years ago by Eric Gourgoulhon

Replying to gh-DeRhamSource:

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 in reply to:  15 Changed 3 years ago by Michael Jung

Replying to egourgoulhon:

Replying to gh-DeRhamSource:

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.

Last edited 3 years ago by Michael Jung (previous) (diff)

comment:17 in reply to:  13 Changed 3 years ago by Eric Gourgoulhon

Replying to gh-DeRhamSource:

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 Michael Jung

sage: M = Manifold(2, 'M') # the 2-dimensional 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 Travis Scrimshaw

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 Michael Jung

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 2-dimensional 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

Last edited 3 years ago by Michael Jung (previous) (diff)

comment:21 Changed 3 years ago by git

Commit: 5d30100a5f1e4f809867dc5b564000cd2eddc9db3f1169874af7db4200bb91544854bb9f90afc9fc

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

3f11698Continuations of zero and one untouched

comment:22 Changed 3 years ago by Michael Jung

Status: needs_infoneeds_review

What about this?

comment:23 Changed 3 years ago by Travis Scrimshaw

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 Michael Jung

Status: needs_reviewneeds_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 Michael Jung

Branch: u/gh-DeRhamSource/better_zero_treatmentu/gh-DeRhamSource/better_zero_treatment_alternative
Commit: 3f1169874af7db4200bb91544854bb9f90afc9fc
Status: needs_workneeds_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.

Last edited 3 years ago by Michael Jung (previous) (diff)

comment:26 Changed 3 years ago by git

Commit: 286553516abfc2895b4136fda99ce7fff653ff4c

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

9f91503'_is_zero' attribute added and modified
5094a3eReturn no copies for scalar field operations
e016b21Zero and one treatment for free tensor modules
371a255Zero treatment for wedge product of free alt forms
1b1ee7fZero/One check added
5bcc7afAlternative solution via unsafe method
2865535Obsolete '**kwargs' removed

comment:27 Changed 3 years ago by Michael Jung

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 git

Commit: 286553516abfc2895b4136fda99ce7fff653ff4cc104fc844d8cd50446944d0cd1b177c369366653

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

c104fc8Scalar field zero treatment

comment:29 Changed 3 years ago by git

Commit: c104fc844d8cd50446944d0cd1b177c369366653efd8e0390aa99ee7d8c667e3120df43ee624e59a

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

efd8e03Mixed form zero treatment

comment:30 Changed 3 years ago by Eric Gourgoulhon

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 git

Commit: efd8e0390aa99ee7d8c667e3120df43ee624e59aa91dba0f83f5bd003f339a834735ea99cd5a510d

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

17e6ff6Merge branch 'develop' into t/28562/better_zero_treatment_alternative
a91dba0Zero if wedge result degree > dim

comment:32 Changed 3 years ago by Eric Gourgoulhon

Branch: u/gh-DeRhamSource/better_zero_treatment_alternativepublic/manifolds/better_zero_treatment
Commit: a91dba0f83f5bd003f339a834735ea99cd5a510d2783253621fa561b0b465ec8fccc30a799b72d88

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 to t.zero() and gl(1) to gl.one() (to skip one function call)
  • some add/set_comp(basis) have been changed to add/set_comp(basis=basis)
  • the treatment of zero has been added in FreeModuleTensor._sub_()

Do you agree with these changes?


New commits:

2783253Minor changes in the zero and one treatment of tensor fields

comment:33 Changed 3 years ago by Michael Jung

Yes, looks good! :)

comment:34 Changed 3 years ago by Travis Scrimshaw

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 Eric Gourgoulhon

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 Changed 3 years ago by Travis Scrimshaw

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 in reply to:  36 Changed 3 years ago by Eric Gourgoulhon

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 git

Commit: 2783253621fa561b0b465ec8fccc30a799b72d88f2928f882a5a8eb345dd11c308e895d615f9de9d

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

f2928f8Blankline removed

comment:39 in reply to:  34 ; Changed 3 years ago by Michael Jung

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 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).

Done.

Last edited 3 years ago by Michael Jung (previous) (diff)

comment:40 in reply to:  39 Changed 3 years ago by Eric Gourgoulhon

Replying to gh-DeRhamSource:

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 Eric Gourgoulhon

Status: needs_reviewpositive_review

Thanks for your work on this!

comment:42 Changed 3 years ago by Eric Gourgoulhon

Reviewers: Eric Gourgoulhon, Travis Scrimshaw

comment:43 Changed 3 years ago by Volker Braun

Branch: public/manifolds/better_zero_treatmentf2928f882a5a8eb345dd11c308e895d615f9de9d
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.