Opened 7 months ago
Last modified 4 months ago
#30239 needs_info defect
TensorField.__call__ alters zero
Reported by: | gh-mjungmath | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.3 |
Component: | manifolds | Keywords: | |
Cc: | egourgoulhon, tscrim, mkoeppe | Merged in: | |
Authors: | Michael Jung | Reviewers: | Eric Gourgoulhon |
Report Upstream: | N/A | Work issues: | |
Branch: | u/gh-mjungmath/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 3-dimensional differentiable manifold M sage: M.zero_scalar_field() Scalar field omega(d/dx) on the 3-dimensional differentiable manifold M
Change History (40)
comment:1 Changed 7 months ago by
- Branch set to u/gh-mjungmath/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 follow-up: ↓ 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 ; follow-up: ↓ 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 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.
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 follow-up: ↓ 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: try-finally instead of try-except
|
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: try-finally instead of try-except"
|
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 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 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 "return-copy-behavior", this ticket would become obsolete. Thus, I hope for a quick response so that I can work further on either one.
comment:37 follow-up: ↓ 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 sage-9.2 to sage-9.3
New commits:
Trac #30239: comp zero check for __call__