Opened 4 months ago

Last modified 5 weeks 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) Commit: 9f633512058835c3a37c353cbc6a735ac021e405
Dependencies: #30266, #30291 Stopgaps:

Description (last modified by egourgoulhon)

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 4 months ago by gh-mjungmath

  • Branch set to u/gh-mjungmath/tensorfield___call___alters_zero

comment:2 Changed 4 months ago by gh-mjungmath

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

New commits:

51ba4e3Trac #30239: comp zero check for __call__

comment:3 Changed 4 months ago by mkoeppe

  • Authors set to Michael Jung

comment:4 follow-up: Changed 4 months 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 4 months ago by egourgoulhon (previous) (diff)

comment:5 Changed 4 months ago by egourgoulhon

  • Authors set to Michael Jung

Sorry the author name was deleted by mistake.

comment:6 Changed 4 months ago by egourgoulhon

  • Description modified (diff)

comment:7 in reply to: ↑ 4 ; follow-up: Changed 4 months ago by 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.

Last edited 4 months ago by gh-mjungmath (previous) (diff)

comment:8 in reply to: ↑ 7 Changed 4 months ago by 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.

comment:9 Changed 4 months ago by git

  • Commit changed from 51ba4e31bed6a7c1febd9fa270a1b56f929064af to 78a50ebde64cfc196e0995427ec6574eebeeff70

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

78a50ebTrac #30329: is zero check

comment:10 Changed 4 months ago by gh-mjungmath

  • Status changed from needs_work to needs_review

What about this?

comment:11 Changed 4 months ago by git

  • Commit changed from 78a50ebde64cfc196e0995427ec6574eebeeff70 to 3344505b364c014baee19c2f4da41c8a7d2bd2f7

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

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

comment:12 Changed 4 months ago by mkoeppe

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

Last edited 4 months ago by mkoeppe (previous) (diff)

comment:13 Changed 4 months 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: Changed 4 months 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 4 months ago by gh-mjungmath

  • Dependencies set to #30261

comment:16 Changed 4 months ago by git

  • Commit changed from 3344505b364c014baee19c2f4da41c8a7d2bd2f7 to d58d291b8bad3a28aae9be2c426f0aa47370f745

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

286d3c6FreeModuleTensor, Vector: Subclass a new class ModuleElementWithMutability
9df3d22sage.tensor.modules: Make all zero() and one() elements immutable
4373ea2FreeModuleTensor: Replace special casing of the immutable zero by is_immutable
a2ee8beModuleElementWithMutability.is_immutable, is_mutable: Change def to cpdef
aebadedTrac #30266: make scalarfields immutable
504e84eTrac #30239: Merge branch 't/30266/immutability_for_scalar_fields' into t/30239/tensorfield___call___alters_zero
d58d291Trac #30266: hash function condition + treatment of restrictions

comment:17 Changed 4 months ago by git

  • Commit changed from d58d291b8bad3a28aae9be2c426f0aa47370f745 to 061b89fc7ced8730d2cd33a13065189872a78ff7

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

061b89fTrac #30266: minor doctest improvement

comment:18 Changed 4 months ago by git

  • Commit changed from 061b89fc7ced8730d2cd33a13065189872a78ff7 to 0ec99d4f357c0eb89ad6e1b12a44810b2ecc764d

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

5c7fd6eTrac #30266: immutability of restrictions + hash function improved
7ba0865Trac #30266: merge
0ec99d4Trac #30266: check by name

comment:19 Changed 4 months ago by git

  • Commit changed from 0ec99d4f357c0eb89ad6e1b12a44810b2ecc764d to 43ec4976081482b1ce467f0ca83e0c73192039f4

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

87fb411Trac #30266: ValueError replaced by AssertionError
43ec497Trac #30239: referenced before assignment error fixed

comment:20 Changed 4 months ago by gh-mjungmath

  • Status changed from needs_work to needs_review

comment:21 Changed 4 months ago by gh-mjungmath

  • Dependencies changed from #30261 to #30266

comment:22 Changed 4 months ago by git

  • Commit changed from 43ec4976081482b1ce467f0ca83e0c73192039f4 to a9cb6b3697e6852fb6d4924e273419bded123a9d

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

a9cb6b3Trac #30239: doctest fixed

comment:23 in reply to: ↑ 14 Changed 4 months ago by 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.

comment:24 Changed 4 months ago by git

  • Commit changed from a9cb6b3697e6852fb6d4924e273419bded123a9d to 731043f54230845c930a4fae1e3241ead02ce641

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

731043fTrac #30239: check by name for general tensor field action

comment:25 Changed 4 months ago by egourgoulhon

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

comment:26 Changed 4 months ago by git

  • Commit changed from 731043f54230845c930a4fae1e3241ead02ce641 to 18480912b7409ca8a0584eccd7ed2ba44c826adc

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

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

comment:27 Changed 4 months ago by gh-mjungmath

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

comment:28 Changed 4 months ago by tscrim

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

comment:29 Changed 4 months 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 4 months ago by git

  • Commit changed from 18480912b7409ca8a0584eccd7ed2ba44c826adc to 4756bc31feaec758d7a1fe50d2b5edc28a71202a

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

4756bc3Revert "Trac #30239: try-finally instead of try-except"

comment:31 Changed 4 months ago by gh-mjungmath

There we go.

comment:32 Changed 4 months ago by git

  • Commit changed from 4756bc31feaec758d7a1fe50d2b5edc28a71202a to 06382070ef75723a2643da5968c86bb5e271806d

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

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

comment:33 Changed 4 months ago by gh-mjungmath

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

comment:34 Changed 4 months ago by git

  • Commit changed from 06382070ef75723a2643da5968c86bb5e271806d to 5c14b3928c13e5cf62f0ab13ebc3b3580fe2b14e

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

2446f7eTrac 30291: simple checks for trivial cases in _mul_, _add_ and _div_
4bd82e4Trac #30291: patchbot doctest fixed
b2ba1e8Merge branch 't/30291/scalar_field_arithmetics__trivial_cases' into t/30239/tensorfield___call___alters_zero
5c14b39Trac #30239: doctest adapted

comment:35 Changed 4 months 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:

2416d87ModuleElementWithMutability.is_immutable, is_mutable: Change def to cpdef
4f07938Trac #30266: make scalarfields immutable
3faa75aTrac #30266: hash function condition + treatment of restrictions
f812586Trac #30266: minor doctest improvement
02726e2Trac #30266: check by name
81e67cfTrac #30266: ValueError replaced by AssertionError
cf4fce5Trac #30239: use try-except block for immutable elements
4f3b69fTrac 30291: simple checks for trivial cases in _mul_, _add_ and _div_
9d8720dMerge branch 't/30291/scalar_field_arithmetics__trivial_cases' into t/30239/tensorfield___call___alters_zero
9f63351Trac #30239: doctest outputs adapted

comment:36 Changed 4 months 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 4 months ago by gh-mjungmath (previous) (diff)

comment:37 follow-up: Changed 4 months 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 4 months ago by 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?

comment:39 Changed 4 months ago by gh-mjungmath

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

comment:40 Changed 5 weeks ago by mkoeppe

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