Opened 2 years ago

TensorField.__call__ alters zero

Reported by: Owned by: gh-mjungmath major sage-9.7 manifolds egourgoulhon, tscrim, mkoeppe Michael Jung Eric Gourgoulhon N/A u/gh-mjungmath/tensorfield___call___alters_zero 9f633512058835c3a37c353cbc6a735ac021e405 #30266, #30291

```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
```

comment:1 Changed 2 years ago by gh-mjungmath

• Branch set to u/gh-mjungmath/tensorfield___call___alters_zero

comment:2 Changed 2 years ago by gh-mjungmath

• Commit set to 51ba4e31bed6a7c1febd9fa270a1b56f929064af
• Status changed from new to needs_review

New commits:

 ​51ba4e3 `Trac #30239: comp zero check for __call__`

comment:3 Changed 2 years ago by mkoeppe

• Authors set to Michael Jung

comment:4 follow-up: ↓ 7 Changed 2 years ago by egourgoulhon

• Authors Michael Jung deleted
• Status changed from needs_review to needs_work

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

Last edited 2 years ago by egourgoulhon (previous) (diff)

comment:5 Changed 2 years ago by egourgoulhon

• Authors set to Michael Jung

Sorry the author name was deleted by mistake.

comment:6 Changed 2 years ago by egourgoulhon

• Description modified (diff)

comment:7 in reply to: ↑ 4 ; follow-up: ↓ 8 Changed 2 years ago by gh-mjungmath

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.

Last edited 2 years ago by gh-mjungmath (previous) (diff)

comment:8 in reply to: ↑ 7 Changed 2 years ago by egourgoulhon

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.

comment:9 Changed 2 years ago by git

• Commit changed from 51ba4e31bed6a7c1febd9fa270a1b56f929064af to 78a50ebde64cfc196e0995427ec6574eebeeff70

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

 ​78a50eb `Trac #30329: is zero check`

comment:10 Changed 2 years ago by gh-mjungmath

• Status changed from needs_work to needs_review

comment:11 Changed 2 years ago by git

• Commit changed from 78a50ebde64cfc196e0995427ec6574eebeeff70 to 3344505b364c014baee19c2f4da41c8a7d2bd2f7

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

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

comment:12 Changed 2 years ago by mkoeppe

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

Last edited 2 years ago by mkoeppe (previous) (diff)

comment:13 Changed 2 years ago by egourgoulhon

• Reviewers set to Eric Gourgoulhon

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

comment:14 follow-up: ↓ 23 Changed 2 years ago by egourgoulhon

• Status changed from needs_review to needs_work

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

comment:15 Changed 2 years ago by gh-mjungmath

• Dependencies set to #30261

comment:16 Changed 2 years ago by git

• Commit changed from 3344505b364c014baee19c2f4da41c8a7d2bd2f7 to d58d291b8bad3a28aae9be2c426f0aa47370f745

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`

comment:17 Changed 2 years ago by git

• Commit changed from d58d291b8bad3a28aae9be2c426f0aa47370f745 to 061b89fc7ced8730d2cd33a13065189872a78ff7

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

 ​061b89f `Trac #30266: minor doctest improvement`

comment:18 Changed 2 years ago by git

• Commit changed from 061b89fc7ced8730d2cd33a13065189872a78ff7 to 0ec99d4f357c0eb89ad6e1b12a44810b2ecc764d

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`

comment:19 Changed 2 years ago by git

• Commit changed from 0ec99d4f357c0eb89ad6e1b12a44810b2ecc764d to 43ec4976081482b1ce467f0ca83e0c73192039f4

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`

comment:20 Changed 2 years ago by gh-mjungmath

• Status changed from needs_work to needs_review

comment:21 Changed 2 years ago by gh-mjungmath

• Dependencies changed from #30261 to #30266

comment:22 Changed 2 years ago by git

• Commit changed from 43ec4976081482b1ce467f0ca83e0c73192039f4 to a9cb6b3697e6852fb6d4924e273419bded123a9d

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

 ​a9cb6b3 `Trac #30239: doctest fixed`

comment:23 in reply to: ↑ 14 Changed 2 years ago by gh-mjungmath

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.

comment:24 Changed 2 years ago by git

• Commit changed from a9cb6b3697e6852fb6d4924e273419bded123a9d to 731043f54230845c930a4fae1e3241ead02ce641

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

 ​731043f `Trac #30239: check by name for general tensor field action`

comment:25 Changed 2 years ago by egourgoulhon

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

comment:26 Changed 2 years ago by git

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

 ​1848091 `Trac #30239: try-finally instead of try-except`

comment:27 Changed 2 years ago by gh-mjungmath

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

comment:28 Changed 2 years ago by tscrim

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

comment:29 Changed 2 years ago by gh-mjungmath

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

comment:30 Changed 2 years ago by git

• Commit changed from 18480912b7409ca8a0584eccd7ed2ba44c826adc to 4756bc31feaec758d7a1fe50d2b5edc28a71202a

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

 ​4756bc3 `Revert "Trac #30239: try-finally instead of try-except"`

There we go.

comment:32 Changed 2 years ago by git

• Commit changed from 4756bc31feaec758d7a1fe50d2b5edc28a71202a to 06382070ef75723a2643da5968c86bb5e271806d

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

comment:33 Changed 2 years ago by gh-mjungmath

• Dependencies changed from #30266 to #30266, #30291

comment:34 Changed 2 years ago by git

• Commit changed from 06382070ef75723a2643da5968c86bb5e271806d to 5c14b3928c13e5cf62f0ab13ebc3b3580fe2b14e

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`

comment:35 Changed 2 years ago by git

• Commit changed from 5c14b3928c13e5cf62f0ab13ebc3b3580fe2b14e to 9f633512058835c3a37c353cbc6a735ac021e405

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`

comment:36 Changed 2 years ago by gh-mjungmath

• Status changed from needs_review to needs_info

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.

Last edited 2 years ago by gh-mjungmath (previous) (diff)

comment:37 follow-up: ↓ 38 Changed 2 years ago by mkoeppe

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

comment:38 in reply to: ↑ 37 Changed 2 years ago by gh-mjungmath

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?

comment:39 Changed 2 years ago by gh-mjungmath

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

comment:40 Changed 22 months ago by mkoeppe

• Milestone changed from sage-9.2 to sage-9.3

comment:41 Changed 17 months ago by mkoeppe

• Milestone changed from sage-9.3 to sage-9.4

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

comment:42 Changed 13 months ago by mkoeppe

• Milestone changed from sage-9.4 to sage-9.5

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

comment:43 Changed 8 months ago by mkoeppe

• Milestone changed from sage-9.5 to sage-9.6

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

comment:44 Changed 5 months ago by mkoeppe

• Milestone changed from sage-9.6 to sage-9.7
Note: See TracTickets for help on using tickets.