Opened 15 months ago

Closed 3 months ago

#30272 closed enhancement (fixed)

Mixed Forms: set_comp, comp

Reported by: gh-mjungmath Owned by:
Priority: major Milestone: sage-9.4
Component: manifolds Keywords:
Cc: egourgoulhon, tscrim, mkoeppe Merged in:
Authors: Michael Jung Reviewers: Eric Gourgoulhon
Report Upstream: N/A Work issues:
Branch: 91fc96f (Commits, GitHub, GitLab) Commit: 91fc96fed615f6070a729c143703907dc227819e
Dependencies: #31654, #30473 Stopgaps:

Status badges

Description (last modified by gh-mjungmath)

In the upcoming changes we want to allow mixed differential forms, among other things, being made immutable (see #30181, #30261).

However, the current implementation would not support that behavior properly, namely:

sage: M = Manifold(2, 'M')
sage: A = M.mixed_form()
sage: a = M.diff_form(2)
sage: A[2] = a
sage: A[2] is a
True

If A would be set immutable and a is not, this would contradict the behavior of an immutable element. Even if a would automatically be set immutable as soon as A had been set immutable, this might happen not on the behalf of the user.

I'd propose a similar approach as I had done in #30208 for bundle connections, and as it is already known for tensor fields, namely by introducing methods set_comp and comp (add_comp would not be necessary). Then each instance representing a homogeneous component is bound only to the mixed differential form.

As always, suggestions and opinions are welcome.

Attachments (1)

Screenshot 2021-06-27 193218.png (7.5 KB) - added by gh-mjungmath 4 months ago.

Download all attachments as: .zip

Change History (34)

comment:1 Changed 15 months ago by gh-mjungmath

  • Cc mkoeppe added

comment:2 Changed 15 months ago by gh-mjungmath

  • Summary changed from Mixed Forms: set_comp, add_comp, comp to Mixed Forms: add_comp, comp

comment:3 Changed 15 months ago by gh-mjungmath

  • Summary changed from Mixed Forms: add_comp, comp to Mixed Forms: set_comp, comp

comment:4 Changed 15 months ago by gh-mjungmath

  • Description modified (diff)

comment:5 Changed 14 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:6 Changed 8 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

comment:7 Changed 6 months ago by gh-mjungmath

  • Dependencies set to #31653

comment:8 Changed 6 months ago by gh-mjungmath

  • Dependencies changed from #31653 to #31654

comment:9 Changed 6 months ago by gh-mjungmath

  • Branch set to u/gh-mjungmath/mixed_forms__set_comp__comp

comment:10 Changed 6 months ago by git

  • Commit set to 69f7bb51b0bd5d0cfe3db416e4e15b3bedc9997a

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

69f7bb5Trac #30272: new standard name for homogeneous components (without tests)

comment:11 Changed 6 months ago by gh-mjungmath

First approach.

What I did:

  • homogeneous components are bound to mixed form and a priori detached from other instances; in particular
    sage: A[0] = f
    sage: A[0] is f
    

yields False now.

  • components are only initialized on demand (not on initialization of the mixed form)
  • by default, homogeneous components are named according to the mixed form's name: A = A_0, A_1, ...
  • set_comp method similar to tensor fields

I tried to maintain as much backwards compatibility as possible. For example, even if copies now, the names are still applied if A[:] = [f, omega, eta] syntax is used. But the long-term idea is to favor set_comp(i)[:] = ... syntax instead of omega[:] = ... plus A[i] = omega syntax.

Comments are appreciated.

comment:12 Changed 6 months ago by git

  • Commit changed from 69f7bb51b0bd5d0cfe3db416e4e15b3bedc9997a to 27aa360aebdcdd632fa62d6d3161d475f3954221

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

27aa360Trac #30272: doctests added

comment:13 Changed 6 months ago by gh-mjungmath

  • Status changed from new to needs_review

Here we go, that should be it. Please take a close look whether this is (at least) halfway consistent and transparent to the user.

In a next step, mixed forms will be equipped with an immutability switch.

comment:14 Changed 6 months ago by gh-mjungmath

If desired so, I will squash the commits for final merge.

comment:15 Changed 6 months ago by gh-mjungmath

  • Authors set to Michael Jung

comment:16 Changed 6 months ago by egourgoulhon

  • Reviewers set to Eric Gourgoulhon
  • Status changed from needs_review to positive_review

LGTM.

comment:17 Changed 4 months ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:18 Changed 4 months ago by git

  • Commit changed from 27aa360aebdcdd632fa62d6d3161d475f3954221 to 89dc5520f76fc7ef937c073e5a799925491a88ea

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

89dc552Trac #30272: merged

comment:19 Changed 4 months ago by gh-mjungmath

  • Status changed from needs_work to needs_review

comment:20 follow-up: Changed 4 months ago by egourgoulhon

When clicking on the new branch, one gets a huge diff, which seems to be the whole 9.4.beta3 w.r.t 9.4.beta2. Wouldn't it be better to merge the previous ticket branch into a clean 9.4.beta3?

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

Changed 4 months ago by gh-mjungmath

comment:21 Changed 4 months ago by gh-mjungmath

That's odd. For me the branch against the current beta looks just fine:

https://trac.sagemath.org/raw-attachment/ticket/30272/Screenshot%202021-06-27%20193218.png

comment:22 in reply to: ↑ 20 ; follow-up: Changed 4 months ago by tscrim

Replying to egourgoulhon:

When clicking on the new branch, one gets a huge diff, which seems to be the whole 9.4.beta3 w.r.t 9.4.beta2. Wouldn't it be better to merge the previous ticket branch into a clean 9.4.beta3?

That sometimes happens with the trac git helper. You don't have to worry too much about it as long as it pulls okay (a la comment:21).

comment:23 in reply to: ↑ 22 Changed 4 months ago by egourgoulhon

Replying to tscrim:

Replying to egourgoulhon:

When clicking on the new branch, one gets a huge diff, which seems to be the whole 9.4.beta3 w.r.t 9.4.beta2. Wouldn't it be better to merge the previous ticket branch into a clean 9.4.beta3?

That sometimes happens with the trac git helper. You don't have to worry too much about it as long as it pulls okay (a la comment:21).

Indeed, this seems erratic: it looks good now, while the branch has not been changed.

comment:24 Changed 4 months ago by egourgoulhon

  • Status changed from needs_review to positive_review

comment:25 Changed 4 months ago by vbraun

  • Status changed from positive_review to needs_work
[release@zen Sage]$ sage -t --long --warn-long 49.3 --random-seed=0 src/sage/manifolds/differentiable/mixed_form.py  # 4 doctests failed
 
Running doctests with ID 2021-06-29-21-56-39-c62006fe.
Git branch: develop
Using --optional=build,dochtml,fedora,pip,sage,sage_spkg
Doctesting 1 file.
sage -t --long --warn-long 49.3 --random-seed=0 src/sage/manifolds/differentiable/mixed_form.py
**********************************************************************
File "src/sage/manifolds/differentiable/mixed_form.py", line 90, in sage.manifolds.differentiable.mixed_form.MixedForm
Failed example:
    A.display_expansion() # display expansion in basis
Expected:
    A = [x] + [x*y dx] + [x*y^2 dx/\dy]
Got:
    A = x + x*y dx + x*y^2 dx/\dy
**********************************************************************
File "src/sage/manifolds/differentiable/mixed_form.py", line 113, in sage.manifolds.differentiable.mixed_form.MixedForm
Failed example:
    B.display_expansion()
Expected:
    B = [x] + [x*y dx] + [x*y^2 dx/\dy]
Got:
    B = x + x*y dx + x*y^2 dx/\dy
**********************************************************************
[...]

comment:26 Changed 4 months ago by gh-mjungmath

Ah damn, I mis-resolved the merge conflict.

Let's wait for #30473 anyway.

comment:27 Changed 4 months ago by gh-mjungmath

  • Dependencies changed from #31654 to #31654, #30473

comment:28 Changed 3 months ago by git

  • Commit changed from 89dc5520f76fc7ef937c073e5a799925491a88ea to f1e7ef884b03cf3ad6a7fe09b0b8892acc818063

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

da8893fUse sage.typeset.unicode_characters in TensorProductFunctor and SignedTensorProductFunctor
2a23cb5Unicode symbol 2202 (partial) for the text display of coordinate frames
5d096f1f-string for unicode_symbol in TensorProductFunctor and SignedTensorProductFunctor
76c2fd5Use Unicode symbol for the Riemann sphere example
5167e6cUse Unicode symbol for default text display of RealLine
332410bUse unicode_otimes in TensorProductFunctor and SignedTensorProductFunctor
f5d15d2Merge branch 'public/manifolds/unicode_art' of git://trac.sagemath.org/sage into Sage 9.4.beta4.
d87d09b#30473: fix doctest error in DiffMap.pullback
d62adadmerge unicode
f1e7ef8Trac #30272: remove set_name_comp + fix doctest

comment:29 Changed 3 months ago by gh-mjungmath

  • Status changed from needs_work to needs_review

comment:30 Changed 3 months ago by git

  • Commit changed from f1e7ef884b03cf3ad6a7fe09b0b8892acc818063 to 91fc96fed615f6070a729c143703907dc227819e

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

91fc96fTrac #30272: minor doctest fix

comment:31 Changed 3 months ago by gh-mjungmath

Ready for review again.

comment:32 Changed 3 months ago by egourgoulhon

  • Status changed from needs_review to positive_review

The patchbot reports doctest failures that all pertain to the dependency #30473. They are all fixed in the latest version of that ticket. But I don't think it necessary to merge it here to move on.

comment:33 Changed 3 months ago by vbraun

  • Branch changed from u/gh-mjungmath/mixed_forms__set_comp__comp to 91fc96fed615f6070a729c143703907dc227819e
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.