Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mixed Forms: set_comp, comp #30272

Closed
mjungmath opened this issue Aug 2, 2020 · 33 comments
Closed

Mixed Forms: set_comp, comp #30272

mjungmath opened this issue Aug 2, 2020 · 33 comments

Comments

@mjungmath
Copy link

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.

Depends on #31654
Depends on #30473

CC: @egourgoulhon @tscrim @mkoeppe

Component: manifolds

Author: Michael Jung

Branch/Commit: 91fc96f

Reviewer: Eric Gourgoulhon

Issue created by migration from https://trac.sagemath.org/ticket/30272

@mjungmath mjungmath added this to the sage-9.2 milestone Aug 2, 2020
@mjungmath mjungmath changed the title Mixed Forms: set_comp, add_comp, comp Mixed Forms: add_comp, comp Aug 2, 2020
@mjungmath mjungmath changed the title Mixed Forms: add_comp, comp Mixed Forms: set_comp, comp Aug 2, 2020
@mjungmath

This comment has been minimized.

@mkoeppe mkoeppe modified the milestones: sage-9.2, sage-9.3 Aug 29, 2020
@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 13, 2021

comment:6

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

@mkoeppe mkoeppe modified the milestones: sage-9.3, sage-9.4 Feb 13, 2021
@mjungmath
Copy link
Author

Dependencies: #31653

@mjungmath
Copy link
Author

Changed dependencies from #31653 to #31654

@mjungmath
Copy link
Author

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 12, 2021

Commit: 69f7bb5

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 12, 2021

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

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

@mjungmath
Copy link
Author

comment:11

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 13, 2021

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

27aa360Trac #30272: doctests added

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 13, 2021

Changed commit from 69f7bb5 to 27aa360

@mjungmath
Copy link
Author

comment:13

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.

@mjungmath
Copy link
Author

comment:14

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

@mjungmath
Copy link
Author

Author: Michael Jung

@egourgoulhon
Copy link
Member

Reviewer: Eric Gourgoulhon

@egourgoulhon
Copy link
Member

comment:16

LGTM.

@vbraun
Copy link
Member

vbraun commented Jun 7, 2021

comment:17

Merge conflict

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 27, 2021

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

89dc552Trac #30272: merged

@egourgoulhon
Copy link
Member

comment:20

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?

@mjungmath
Copy link
Author

Attachment: Screenshot 2021-06-27 193218.png

@mjungmath
Copy link
Author

comment:21

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

@tscrim
Copy link
Collaborator

tscrim commented Jun 27, 2021

comment:22

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

@egourgoulhon
Copy link
Member

comment:23

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.

@vbraun
Copy link
Member

vbraun commented Jun 29, 2021

comment:25
[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
**********************************************************************
[...]

@mjungmath
Copy link
Author

comment:26

Ah damn, I mis-resolved the merge conflict.

Let's wait for #30473 anyway.

@mjungmath
Copy link
Author

Changed dependencies from #31654 to #31654, #30473

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 6, 2021

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 6, 2021

Changed commit from 89dc552 to f1e7ef8

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 6, 2021

Changed commit from f1e7ef8 to 91fc96f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 6, 2021

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

91fc96fTrac #30272: minor doctest fix

@mjungmath
Copy link
Author

comment:31

Ready for review again.

@egourgoulhon
Copy link
Member

comment:32

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.

@vbraun
Copy link
Member

vbraun commented Jul 23, 2021

Changed branch from u/gh-mjungmath/mixed_forms__set_comp__comp to 91fc96f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants