TensorField.__call__ alters zero

[gh-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

[gh-mjungmath]

New commits:

​51ba4e3

Trac #30239: comp zero check for __call__

[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. 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]

Sorry the author name was deleted by mistake.

[gh-mjungmath]

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]

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

[git]

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

​78a50eb

Trac #30329: is zero check

[gh-mjungmath]

What about this?

[git]

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

​3344505

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

[mkoeppe]

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

[egourgoulhon]

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

[egourgoulhon]

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.

[git]

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

​286d3c6	FreeModuleTensor, Vector: Subclass a new class ModuleElementWithMutability
​9df3d22	sage.tensor.modules: Make all zero() and one() elements immutable
​4373ea2	FreeModuleTensor: Replace special casing of the immutable zero by is_immutable
​a2ee8be	ModuleElementWithMutability.is_immutable, is_mutable: Change def to cpdef
​aebaded	Trac #30266: make scalarfields immutable
​504e84e	Trac #30239: Merge branch 't/30266/immutability_for_scalar_fields' into t/30239/tensorfield___call___alters_zero
​d58d291	Trac #30266: hash function condition + treatment of restrictions

[git]

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

​061b89f

Trac #30266: minor doctest improvement

[git]

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

​5c7fd6e	Trac #30266: immutability of restrictions + hash function improved
​7ba0865	Trac #30266: merge
​0ec99d4	Trac #30266: check by name

[git]

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

​87fb411	Trac #30266: ValueError replaced by AssertionError
​43ec497	Trac #30239: referenced before assignment error fixed

[git]

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

​a9cb6b3

Trac #30239: doctest fixed

[gh-mjungmath]

Replying to egourgoulhon:

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.

Both have been fixed. Wait for green bot.

[git]

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

​731043f

Trac #30239: check by name for general tensor field action

[egourgoulhon]

LGTM. Waiting for a positive review of the dependency (#30266).

[git]

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

​1848091

Trac #30239: try-finally instead of try-except

[gh-mjungmath]

What about try...finally instead of try...except? I think this should be even faster, right?

[tscrim]

I doubt it. Plus it makes the code a little harder to understand IMO.

[gh-mjungmath]

You're right, there is no difference. And I agree with you regarding the readability. I'll change it back.

[git]

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

​4756bc3

Revert "Trac #30239: try-finally instead of try-except"

[gh-mjungmath]

There we go.

[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_

[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

[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

[gh-mjungmath]

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]

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

[gh-mjungmath]

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?

[gh-mjungmath]

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

[mkoeppe]

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]

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

[mkoeppe]

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