Opened 7 months ago
Last modified 4 months ago
#30239 needs_info defect
TensorField.__call__ alters zero
Reported by:  ghmjungmath  Owned by:  

Priority:  major  Milestone:  sage9.3 
Component:  manifolds  Keywords:  
Cc:  egourgoulhon, tscrim, mkoeppe  Merged in:  
Authors:  Michael Jung  Reviewers:  Eric Gourgoulhon 
Report Upstream:  N/A  Work issues:  
Branch:  u/ghmjungmath/tensorfield___call___alters_zero (Commits, GitHub, GitLab)  Commit:  9f633512058835c3a37c353cbc6a735ac021e405 
Dependencies:  #30266, #30291  Stopgaps: 
Description (last modified by )
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 3dimensional differentiable manifold M sage: M.zero_scalar_field() Scalar field omega(d/dx) on the 3dimensional differentiable manifold M
Change History (40)
comment:1 Changed 7 months ago by
 Branch set to u/ghmjungmath/tensorfield___call___alters_zero
comment:2 Changed 7 months ago by
 Commit set to 51ba4e31bed6a7c1febd9fa270a1b56f929064af
 Status changed from new to needs_review
comment:3 Changed 7 months ago by
comment:4 followup: ↓ 7 Changed 7 months ago by
 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).
comment:6 Changed 7 months ago by
 Description modified (diff)
comment:7 in reply to: ↑ 4 ; followup: ↓ 8 Changed 7 months ago by
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.
comment:8 in reply to: ↑ 7 Changed 7 months ago by
Replying to ghmjungmath:
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 7 months ago by
 Commit changed from 51ba4e31bed6a7c1febd9fa270a1b56f929064af to 78a50ebde64cfc196e0995427ec6574eebeeff70
Branch pushed to git repo; I updated commit sha1. New commits:
78a50eb  Trac #30329: is zero check

comment:11 Changed 7 months ago by
 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 7 months ago by
I think #30181 (Immutable elements of FreeModuleTensor
) could provide a systematic solution
comment:13 Changed 7 months ago by
 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 followup: ↓ 23 Changed 7 months ago by
 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 7 months ago by
 Dependencies set to #30261
comment:16 Changed 7 months ago by
 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 7 months ago by
 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 7 months ago by
 Commit changed from 061b89fc7ced8730d2cd33a13065189872a78ff7 to 0ec99d4f357c0eb89ad6e1b12a44810b2ecc764d
comment:19 Changed 7 months ago by
 Commit changed from 0ec99d4f357c0eb89ad6e1b12a44810b2ecc764d to 43ec4976081482b1ce467f0ca83e0c73192039f4
comment:20 Changed 7 months ago by
 Status changed from needs_work to needs_review
comment:21 Changed 7 months ago by
 Dependencies changed from #30261 to #30266
comment:22 Changed 7 months ago by
 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 7 months ago by
Replying to egourgoulhon:
There is a doctest error in
src/sage/manifolds/differentiable/chart.py
and some pycodestyle error insrc/sage/tensor/modules/free_module_tensor.py
.
Both have been fixed. Wait for green bot.
comment:24 Changed 7 months ago by
 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 7 months ago by
LGTM. Waiting for a positive review of the dependency (#30266).
comment:26 Changed 7 months ago by
 Commit changed from 731043f54230845c930a4fae1e3241ead02ce641 to 18480912b7409ca8a0584eccd7ed2ba44c826adc
Branch pushed to git repo; I updated commit sha1. New commits:
1848091  Trac #30239: tryfinally instead of tryexcept

comment:27 Changed 7 months ago by
What about try...finally
instead of try...except
? I think this should be even faster, right?
comment:28 Changed 7 months ago by
I doubt it. Plus it makes the code a little harder to understand IMO.
comment:29 Changed 7 months ago by
You're right, there is no difference. And I agree with you regarding the readability. I'll change it back.
comment:30 Changed 7 months ago by
 Commit changed from 18480912b7409ca8a0584eccd7ed2ba44c826adc to 4756bc31feaec758d7a1fe50d2b5edc28a71202a
Branch pushed to git repo; I updated commit sha1. New commits:
4756bc3  Revert "Trac #30239: tryfinally instead of tryexcept"

comment:31 Changed 7 months ago by
There we go.
comment:32 Changed 7 months ago by
 Commit changed from 4756bc31feaec758d7a1fe50d2b5edc28a71202a to 06382070ef75723a2643da5968c86bb5e271806d
comment:33 Changed 7 months ago by
 Dependencies changed from #30266 to #30266, #30291
comment:34 Changed 7 months ago by
 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 7 months ago by
 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 tryexcept 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 7 months ago by
 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 "returncopybehavior", this ticket would become obsolete. Thus, I hope for a quick response so that I can work further on either one.
comment:37 followup: ↓ 38 Changed 7 months ago by
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 7 months ago by
Replying to mkoeppe:
FWIW,
FreeModule
returns a mutable copy even on trivial operations such asw = 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 7 months ago by
I've opened another ticket #30302 for this discussion.
comment:40 Changed 4 months ago by
 Milestone changed from sage9.2 to sage9.3
New commits:
Trac #30239: comp zero check for __call__