TensorField.__call__ alters zero

[mjungmath]

sage: M = Manifold(3, 'M', start_index=1)
sage: X.<x,y,z> = M.chart()
sage: omega = M.diff_form(1, 'omega')
sage: omega[:] = [0, 0, 0]
sage: omega(X.frame()[1])
Scalar field omega(d/dx) on the 3-dimensional differentiable manifold M
sage: M.zero_scalar_field()
Scalar field omega(d/dx) on the 3-dimensional differentiable manifold M

Depends on #30266
Depends on #30291

CC: @egourgoulhon @tscrim @mkoeppe

Component: manifolds

Author: Michael Jung

Branch/Commit: u/gh-mjungmath/tensorfield___call___alters_zero @ 9f63351

Reviewer: Eric Gourgoulhon

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

[mjungmath]

Branch: u/gh-mjungmath/tensorfield___call___alters_zero

[mjungmath]

Commit: 51ba4e3

[mjungmath]

New commits:

51ba4e3

Trac #30239: comp zero check for __call__

[mkoeppe]

Author: Michael Jung

[egourgoulhon]

comment:4

Good catch!

However the proposed fix

            if omega == 0 or vv == 0:
                return self.base_ring().zero()

is not acceptable, because the comparison with 0 can be very slow for complicated expressions.
Also, this does not fix the root of the bug, which results from the += operator acting on the zero element. It should be fixed per se, because it can lead to other errors.
FWIW, this bug was introduced in Sage 9.0 (everything is OK in earlier versions of Sage).

[egourgoulhon]

Changed author from Michael Jung to none

[egourgoulhon]

comment:5

Sorry the author name was deleted by mistake.

[egourgoulhon]

Author: Michael Jung

[mjungmath]

comment:7

Replying to @egourgoulhon:

Good catch!

However the proposed fix

            if omega == 0 or vv == 0:
                return self.base_ring().zero()

is not acceptable, because the comparison with 0 can be very slow for complicated expressions.

I was afraid that you say that. :D
I haven't found a fast check for components. Is there any?

Also, this does not fix the root of the bug, which results from the += operator acting on the zero element. It should be fixed per se, because it can lead to other errors.
FWIW, this bug was introduced in Sage 9.0 (everything is OK in earlier versions of Sage).

I suspect the root is #28562. However, the operator += should not be a problem here. When two elements are added from which one is the zero element, the other element is returned. Due to ticket #28562, altering the components of the zero element is not even possible and would result in an error. The only thing happening here is that the zero element is renamed.

[egourgoulhon]

comment:8

Replying to @mjungmath:

I was afraid that you say that. :D
I haven't found a fast check for components. Is there any?

No, is_trivial_zero is not implemented there.

Also, this does not fix the root of the bug, which results from the += operator acting on the zero element. It should be fixed per se, because it can lead to other errors.
FWIW, this bug was introduced in Sage 9.0 (everything is OK in earlier versions of Sage).

I suspect the root is #28562. However, the operator += should not be a problem here. When two elements are added from which one is the zero element, the other element is returned. Due to ticket #28562, altering the components of the zero element is not even possible and would result in an error. The only thing happening here is that the zero element is renamed.

You are right, += is not the problem here. A way to fix the issue is then to check whether the computed result is the zero of the base ring (I guess by something like resu is self._fmodule.base().zero()), before changing its name at the end of the __call__ method.

[sagetrac-git]

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

78a50eb

Trac #30329: is zero check

[sagetrac-git]

Changed commit from 51ba4e3 to 78a50eb

[mjungmath]

comment:10

What about this?

[sagetrac-git]

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

3344505

Trac #30239: one check for (0,1) tensors

[sagetrac-git]

Changed commit from 78a50eb to 3344505

[mkoeppe]

comment:12

I think #30181 (Immutable elements of FreeModuleTensor) could provide a systematic solution

[egourgoulhon]

Reviewer: Eric Gourgoulhon

[egourgoulhon]

comment:13

Thanks for covering the case of one() as well. Seems OK to me. Waiting for the patchbot green light...

[egourgoulhon]

comment:14

There is a doctest error in src/sage/manifolds/differentiable/chart.py and some pycodestyle error in src/sage/tensor/modules/free_module_tensor.py.

[sagetrac-git]

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

abda8ec	Merge branch 'develop' into t/30239/tensorfield___call___alters_zero
0638207	Trac 30291: simple checks for trivial cases in `_mul_`, `_add_` and _div_

[mjungmath]

Changed dependencies from #30266 to #30266, #30291

[sagetrac-git]

Changed commit from 0638207 to 5c14b39

[sagetrac-git]

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

2446f7e	Trac 30291: simple checks for trivial cases in `_mul_`, `_add_` and _div_
4bd82e4	Trac #30291: patchbot doctest fixed
b2ba1e8	Merge branch 't/30291/scalar_field_arithmetics__trivial_cases' into t/30239/tensorfield___call___alters_zero
5c14b39	Trac #30239: doctest adapted

[sagetrac-git]

Changed commit from 5c14b39 to 9f63351

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

2416d87	ModuleElementWithMutability.is_immutable, is_mutable: Change def to cpdef
4f07938	Trac #30266: make scalarfields immutable
3faa75a	Trac #30266: hash function condition + treatment of restrictions
f812586	Trac #30266: minor doctest improvement
02726e2	Trac #30266: check by name
81e67cf	Trac #30266: ValueError replaced by AssertionError
cf4fce5	Trac #30239: use try-except block for immutable elements
4f3b69f	Trac 30291: simple checks for trivial cases in `_mul_`, `_add_` and _div_
9d8720d	Merge branch 't/30291/scalar_field_arithmetics__trivial_cases' into t/30239/tensorfield___call___alters_zero
9f63351	Trac #30239: doctest outputs adapted

[mjungmath]

comment:36

Changing the doctest outputs, especially in src/doc/en/thematic_tutorials/vector_calculus/vector_calc_advanced.rst left my aesthetic devil inside of me rebelling. A uniform output would be indeed nice.

If I remember correctly, our original argument for this behavior was that elements of modules/rings are usually treated immutable and that we should recover this behavior as far as possible. But thanks to Matthias with #30181, and the resulting changes in #30261, our elements are now undoubtfully viewed as mutable elements until they are stated immutable.

So, what about keeping the checks and always returning copies after each operation instead? This has also the great advantage that the results are again mutable (i.e. consistent behavior). Since we use fast checks that don't depend on particular elements now, this should also not affect the performance and would fit into the framework.

If we decide for the "return-copy-behavior", this ticket would become obsolete. Thus, I hope for a quick response so that I can work further on either one.

[mkoeppe]

comment:37

FWIW, FreeModule returns a mutable copy even on trivial operations such as w = v + 0, so implementing this behavior would be compatible.

[mjungmath]

comment:38

Replying to @mkoeppe:

FWIW, FreeModule returns a mutable copy even on trivial operations such as w = v + 0, so implementing this behavior would be compatible.

That's good to know. Thanks. Do you also have an opinion which behavior is more suitable?

[mjungmath]

comment:39

I've opened another ticket #30302 for this discussion.

[mkoeppe]

comment:41

Moving this ticket to 9.4, as it seems unlikely that it will be merged in 9.3, which is in the release candidate stage

[mkoeppe]

comment:42

Setting a new milestone for this ticket based on a cursory review.

[mkoeppe]

comment:43

Stalled in needs_review or needs_info; likely won't make it into Sage 9.5.