#28562 closed enhancement (fixed)

Tensor Fields: Better Zero Treatment

Reported by: gh-DeRhamSource Owned by:
Priority: major Milestone: sage-9.0
Component: geometry Keywords: manifolds, tensor fields, scalar fields, mixed forms
Cc: tscrim, egourgoulhon Merged in:
Authors: Michael Jung Reviewers: Eric Gourgoulhon, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: f2928f8 (Commits) Commit: f2928f882a5a8eb345dd11c308e895d615f9de9d
Dependencies: Stopgaps:

Description (last modified by gh-DeRhamSource)

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 12 months ago by gh-DeRhamSource

  • Branch set to u/gh-DeRhamSource/better_zero_treatment

comment:2 Changed 12 months ago by gh-DeRhamSource

  • Commit set to 9f91503dce5cbe14f52a6a77a34ae5a18f722f7d
  • Description modified (diff)

New commits:

9f91503'_is_zero' attribute added and modified

comment:3 Changed 12 months ago by git

  • Commit changed from 9f91503dce5cbe14f52a6a77a34ae5a18f722f7d to 5094a3e802034be5916879a0225e5db2ccf9f0f8

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

5094a3eReturn no copies for scalar field operations

comment:4 Changed 12 months ago by gh-DeRhamSource

  • Description modified (diff)

comment:5 Changed 12 months ago by gh-DeRhamSource

  • Description modified (diff)

comment:6 Changed 12 months ago by git

  • Commit changed from 5094a3e802034be5916879a0225e5db2ccf9f0f8 to 94cf06ab1ab5d87fbc8efc73d2688ea1c8c91267

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 12 months ago by gh-DeRhamSource

  • Component changed from PLEASE CHANGE to geometry
  • Status changed from new to needs_review

comment:8 Changed 12 months ago by gh-DeRhamSource

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 12 months ago by gh-DeRhamSource (previous) (diff)

comment:9 Changed 12 months ago by gh-DeRhamSource

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 12 months ago by git

  • Commit changed from 94cf06ab1ab5d87fbc8efc73d2688ea1c8c91267 to d949a13db29334e193ec9ff05fd7fe53deb65422

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

d949a13Minor code reduction

comment:11 Changed 12 months ago by git

  • Commit changed from d949a13db29334e193ec9ff05fd7fe53deb65422 to 5d30100a5f1e4f809867dc5b564000cd2eddc9db

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

5d30100More code reduction

comment:12 Changed 12 months ago by gh-DeRhamSource

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 12 months ago by gh-DeRhamSource (previous) (diff)

comment:13 follow-ups: Changed 12 months ago by gh-DeRhamSource

  • Status changed from needs_review to 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.

Last edited 12 months ago by gh-DeRhamSource (previous) (diff)

comment:14 Changed 12 months ago by gh-DeRhamSource

  • Description modified (diff)

comment:15 in reply to: ↑ 13 ; follow-up: Changed 12 months ago by 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?

comment:16 in reply to: ↑ 15 Changed 12 months ago by gh-DeRhamSource

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 12 months ago by gh-DeRhamSource (previous) (diff)

comment:17 in reply to: ↑ 13 Changed 12 months ago by egourgoulhon

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 11 months ago by gh-DeRhamSource

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 11 months ago by tscrim

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 11 months ago by gh-DeRhamSource

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 11 months ago by gh-DeRhamSource (previous) (diff)

comment:21 Changed 11 months ago by git

  • Commit changed from 5d30100a5f1e4f809867dc5b564000cd2eddc9db to 3f1169874af7db4200bb91544854bb9f90afc9fc

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

3f11698Continuations of zero and one untouched

comment:22 Changed 11 months ago by gh-DeRhamSource

  • Status changed from needs_info to needs_review

What about this?

comment:23 Changed 11 months ago by tscrim

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 11 months ago by gh-DeRhamSource

  • Status changed from needs_review to 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 11 months ago by gh-DeRhamSource

  • Branch changed from u/gh-DeRhamSource/better_zero_treatment to u/gh-DeRhamSource/better_zero_treatment_alternative
  • Commit 3f1169874af7db4200bb91544854bb9f90afc9fc deleted
  • Status changed from needs_work to 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.

Last edited 11 months ago by gh-DeRhamSource (previous) (diff)

comment:26 Changed 11 months ago by git

  • Commit set to 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 11 months ago by gh-DeRhamSource

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 11 months ago by git

  • Commit changed from 286553516abfc2895b4136fda99ce7fff653ff4c to c104fc844d8cd50446944d0cd1b177c369366653

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

c104fc8Scalar field zero treatment

comment:29 Changed 11 months ago by git

  • Commit changed from c104fc844d8cd50446944d0cd1b177c369366653 to efd8e0390aa99ee7d8c667e3120df43ee624e59a

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

efd8e03Mixed form zero treatment

comment:30 Changed 11 months ago by egourgoulhon

I just gave a quick look at the latest version. Looks good. I'll have a deeper look tomorrow.

comment:31 Changed 11 months ago by git

  • Commit changed from efd8e0390aa99ee7d8c667e3120df43ee624e59a to a91dba0f83f5bd003f339a834735ea99cd5a510d

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 11 months ago by egourgoulhon

  • Branch changed from u/gh-DeRhamSource/better_zero_treatment_alternative to public/manifolds/better_zero_treatment
  • Commit changed from a91dba0f83f5bd003f339a834735ea99cd5a510d to 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 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 11 months ago by gh-DeRhamSource

Yes, looks good! :)

comment:34 follow-up: Changed 11 months ago by 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::

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 11 months ago by egourgoulhon

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 follow-up: Changed 11 months ago by tscrim

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 11 months ago by egourgoulhon

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 11 months ago by git

  • Commit changed from 2783253621fa561b0b465ec8fccc30a799b72d88 to f2928f882a5a8eb345dd11c308e895d615f9de9d

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

f2928f8Blankline removed

comment:39 in reply to: ↑ 34 ; follow-up: Changed 11 months ago by 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.

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 11 months ago by gh-DeRhamSource (previous) (diff)

comment:40 in reply to: ↑ 39 Changed 11 months ago by egourgoulhon

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 11 months ago by egourgoulhon

  • Status changed from needs_review to positive_review

Thanks for your work on this!

comment:42 Changed 11 months ago by egourgoulhon

  • Reviewers set to Eric Gourgoulhon, Travis Scrimshaw

comment:43 Changed 11 months ago by vbraun

  • Branch changed from public/manifolds/better_zero_treatment to f2928f882a5a8eb345dd11c308e895d615f9de9d
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.