search

sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.45k stars 482 forks source link

Tensor Fields: Better Zero Treatment #28562

Closed b220a61f-8dcb-4d62-9a53-d56a2b1bfc8f closed 5 years ago

b220a61f-8dcb-4d62-9a53-d56a2b1bfc8f commented 5 years ago

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

CC: @tscrim @egourgoulhon

Component: geometry

Keywords: tensor fields, scalar fields, mixed forms

Author: Michael Jung

Branch/Commit: f2928f8

Reviewer: Eric Gourgoulhon, Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/28562

b220a61f-8dcb-4d62-9a53-d56a2b1bfc8f commented 5 years ago

Branch: u/gh-DeRhamSource/better_zero_treatment

b220a61f-8dcb-4d62-9a53-d56a2b1bfc8f commented 5 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,9 @@
 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
b220a61f-8dcb-4d62-9a53-d56a2b1bfc8f commented 5 years ago

New commits:

9f91503'_is_zero' attribute added and modified
b220a61f-8dcb-4d62-9a53-d56a2b1bfc8f commented 5 years ago

Commit: 9f91503

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

5094a3eReturn no copies for scalar field operations
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 9f91503 to 5094a3e

b220a61f-8dcb-4d62-9a53-d56a2b1bfc8f commented 5 years ago

Description changed:

--- 
+++ 
@@ -7,3 +7,4 @@
 - 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
b220a61f-8dcb-4d62-9a53-d56a2b1bfc8f commented 5 years ago

Description changed:

--- 
+++ 
@@ -8,3 +8,4 @@
 - 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
+- `_is_zero` attribute is applied for copies
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 5094a3e to 94cf06a

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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
b220a61f-8dcb-4d62-9a53-d56a2b1bfc8f commented 5 years ago
comment:8

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

b220a61f-8dcb-4d62-9a53-d56a2b1bfc8f commented 5 years ago
comment:9

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.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 94cf06a to d949a13

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

d949a13Minor code reduction
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

5d30100More code reduction
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from d949a13 to 5d30100

b220a61f-8dcb-4d62-9a53-d56a2b1bfc8f commented 5 years ago
comment:12

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.

b220a61f-8dcb-4d62-9a53-d56a2b1bfc8f commented 5 years ago
comment:13

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.

b220a61f-8dcb-4d62-9a53-d56a2b1bfc8f commented 5 years ago

Description changed:

--- 
+++ 
@@ -7,5 +7,5 @@
 - 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
+- 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
egourgoulhon commented 5 years ago
comment:15

Replying to @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?

b220a61f-8dcb-4d62-9a53-d56a2b1bfc8f commented 5 years ago
comment:16

Replying to @egourgoulhon:

Replying to @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.

egourgoulhon commented 5 years ago
comment:17

Replying to @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.

b220a61f-8dcb-4d62-9a53-d56a2b1bfc8f commented 5 years ago
comment:18
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?

tscrim commented 5 years ago
comment:19

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.

b220a61f-8dcb-4d62-9a53-d56a2b1bfc8f commented 5 years ago
comment:20

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

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

3f11698Continuations of zero and one untouched
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 5d30100 to 3f11698

b220a61f-8dcb-4d62-9a53-d56a2b1bfc8f commented 5 years ago
comment:22

What about this?

tscrim commented 5 years ago
comment:23

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.

b220a61f-8dcb-4d62-9a53-d56a2b1bfc8f commented 5 years ago
comment:24

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.

b220a61f-8dcb-4d62-9a53-d56a2b1bfc8f commented 5 years ago

Changed commit from 3f11698 to none

b220a61f-8dcb-4d62-9a53-d56a2b1bfc8f commented 5 years ago
comment:25

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.

b220a61f-8dcb-4d62-9a53-d56a2b1bfc8f commented 5 years ago

Changed branch from u/gh-DeRhamSource/better_zero_treatment to u/gh-DeRhamSource/better_zero_treatment_alternative

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Commit: 2865535

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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
b220a61f-8dcb-4d62-9a53-d56a2b1bfc8f commented 5 years ago
comment:27

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?

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

c104fc8Scalar field zero treatment
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 2865535 to c104fc8

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

efd8e03Mixed form zero treatment
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from c104fc8 to efd8e03

egourgoulhon commented 5 years ago
comment:30

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

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from efd8e03 to a91dba0

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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
egourgoulhon commented 5 years ago
comment:32

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

Do you agree with these changes?

New commits:

2783253Minor changes in the zero and one treatment of tensor fields
egourgoulhon commented 5 years ago

Changed commit from a91dba0 to 2783253

egourgoulhon commented 5 years ago

Changed branch from u/gh-DeRhamSource/better_zero_treatment_alternative to public/manifolds/better_zero_treatment

b220a61f-8dcb-4d62-9a53-d56a2b1bfc8f commented 5 years ago
comment:33

Yes, looks good! :)

tscrim commented 5 years ago
comment:34

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

egourgoulhon commented 5 years ago
comment:35

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?

tscrim commented 5 years ago
comment:36

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.