Opened 4 months ago

Closed 4 months ago

#30208 closed enhancement (fixed)

List Assignment for Bundle Connections

Reported by: gh-mjungmath Owned by:
Priority: major Milestone: sage-9.2
Component: manifolds Keywords:
Cc: egourgoulhon, tscrim Merged in:
Authors: Michael Jung Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: d3c8cab (Commits) Commit: d3c8cab917f04ebc9951d41a0cc8dcf4c66392a6
Dependencies: #30191 Stopgaps:

Description

Bundle connections were implementet quite superficially (by me). In this ticket, I want to improve the assigment behavior of bundle connections.

First of all, the methods set_connection_form and add_connection_form now return the corresponding connection 1-form ready for further assignment.

The main improvement in this ticket is to allow list assignments:

sage: M = Manifold(2, 'M', start_index=1)
sage: X.<x,y> = M.chart()
sage: E = M.vector_bundle(2, 'E')
sage: nab = E.bundle_connection('nabla')
sage: e = E.local_frame('e')
sage: nab[e, :] = 0  # set all entries to zero
sage: nab[e, 0, 0].display()
connection (1,1) of bundle connection nabla w.r.t. Local frame (E|_M, (e_1,e_2)) = 0

For details see the documentation, which I have also improved.

Change History (25)

comment:1 Changed 4 months ago by gh-mjungmath

  • Dependencies set to #30191

comment:2 Changed 4 months ago by gh-mjungmath

  • Branch set to u/gh-mjungmath/bundle_connection_extension

comment:3 Changed 4 months ago by gh-mjungmath

  • Commit set to fa87f4b3f439fd02cbef4622efcebb11da329d5f
  • Status changed from new to needs_review

Last 10 new commits:

fe43915Trac #30191: code improvements + in ZZ check removed again
461ff65Trac #30191: != [] removal reverted
af5af8fTrac #30191: elif reverted + in ZZ check for automorphismfield_group
f0c6ee8Trac #30191: minor fix
f1221daTrac #30191: minor fix
a042313Trac# 30191: wrong indentation reverted
4cc92e9Trac #30191: comp check unified to if comp
f98a47bTrac #30191: unused ScalarField import removed
9c5a25bTrac #30208: assigment for bundle connections
fa87f4bTrac #30208: documentation improved

comment:4 Changed 4 months ago by git

  • Commit changed from fa87f4b3f439fd02cbef4622efcebb11da329d5f to 4f0c279585c38920b442426c081bf93ecc526034

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

4f0c279Trac #30208: Merge branch 'develop' into bundle_connection_extension

comment:5 Changed 4 months ago by tscrim

Misindent:

-            sage: M = Manifold(2, 'M')
+             sage: M = Manifold(2, 'M')
             sage: X.<x,y> = M.chart()
             sage: E = M.vector_bundle(2, 'E')

Also in add_connection_form and set_connection_form, you need to deprecate the form input (or keep it with a default of None).

comment:6 Changed 4 months ago by git

  • Commit changed from 4f0c279585c38920b442426c081bf93ecc526034 to 41983daf47bc0ab84485cc19a0b25f582465c343

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

41983daTrac #30208: form input deprecated

comment:7 Changed 4 months ago by gh-mjungmath

Is this suitable?

Version 0, edited 4 months ago by gh-mjungmath (next)

comment:8 follow-up: Changed 4 months ago by tscrim

You should make it a standard deprecation using from sage.misc.superseded import deprecation. Also, there will need to be separate deprecations for each of the two methods.

My (not so well informed) opinion is that both approaches seem natural. So I don't really can't make a comparison about which is better.

comment:9 in reply to: ↑ 8 ; follow-up: Changed 4 months ago by gh-mjungmath

Replying to tscrim:

You should make it a standard deprecation using from sage.misc.superseded import deprecation. Also, there will need to be separate deprecations for each of the two methods.

I will change this. Thank you.

My (not so well informed) opinion is that both approaches seem natural. So I don't really can't make a comparison about which is better.

I thought this approach is more natural in terms of the "immutability" discussion we had months ago. The connection forms are now somehow fixed and clearly bound to the connection. Furthermore, it is much closer to the implementation of AffineConnection.

Eric, what do you say?

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

Replying to gh-mjungmath:

Replying to tscrim:

My (not so well informed) opinion is that both approaches seem natural. So I don't really can't make a comparison about which is better.

I thought this approach is more natural in terms of the "immutability" discussion we had months ago. The connection forms are now somehow fixed and clearly bound to the connection.

A few months ago seems like a lifetime now. Although I think it was more than a few months, but I don't quite remember the outcome of that conversation...

comment:11 in reply to: ↑ 10 Changed 4 months ago by gh-mjungmath

Replying to tscrim:

A few months ago seems like a lifetime now. Although I think it was more than a few months, but I don't quite remember the outcome of that conversation...

And still, time flies...I think the outcome was #28562.

comment:12 Changed 4 months ago by git

  • Commit changed from 41983daf47bc0ab84485cc19a0b25f582465c343 to f9cb6c14c1709302fdbf2d37bae69a0ae098ebe6

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

f9cb6c1Trac #30208: deprecation warning

comment:13 Changed 4 months ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

LGTM.

comment:14 Changed 4 months ago by gh-mjungmath

Thank you for the positive review. Besides, I am still curious what Eric has to say about these changes.

comment:15 Changed 4 months ago by gh-mjungmath

  • Status changed from positive_review to needs_work

Sorry, I have found some severe flaws.

comment:16 Changed 4 months ago by gh-mjungmath

I have sensed something really odd, and I have no idea where this might come from:

sage: M = Manifold(3, 'M', start_index=1)
sage: X.<x,y,z> = M.chart()
sage: E = M.vector_bundle(2, 'E')
sage: e = E.local_frame('e') # standard frame for E
sage: nab = E.bundle_connection('nabla', latex_name=r'\nabla')
sage: nab[e, :] = 0
sage: nab[e, 1, 2][:] = [x*z, y*z, z^2]
sage: nab[e, 2, 1][:] = [x, x^2, x^3]
sage: nab[e, 1, 1][:] = [x+z, y-z, x*y*z]
sage: vframe = X.frame()
sage: e[1][[e, 2]]
0
sage: om = nab[2, 2]
sage: om(vframe[1])
Scalar field connection (2,2) of bundle connection nabla w.r.t. Local frame (E|_M, (e_1,e_2))(d/dx) on the 3-dimensional differentiable manifold M
sage: e[1][[e, 2]]
Scalar field connection (2,2) of bundle connection nabla w.r.t. Local frame (E|_M, (e_1,e_2))(d/dx) on the 3-dimensional differentiable manifold M
sage: e[1][[e, 2]] is om(vframe[1])
True

What the...?

Addendum: I think the problem comes from the unique zero scalar field. Somehow it gets altered via __call__ of TensorField. It has therefore nothing to do with this ticket. I have opened another ticket #30239.

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

comment:17 Changed 4 months ago by git

  • Commit changed from f9cb6c14c1709302fdbf2d37bae69a0ae098ebe6 to bb476e24da2e55f7133e4d9ff14c92a5792401f9

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

c34e481Trac #30191: remove in ZZ check
65dff3dTrac #30191: Merge branch 'develop' into conversion_attributeerror_fix
acc407cTrac #30191: try-except block for zero check
0d27b57Trac #30191: unnecessary comment block removed
f94bcbeTrac #30191: if blocks rearranged
28dec69Trac #30191: doctest errors fixed
6a55a0bMerge branch 'conversion_attributeerror_fix' into bundle_connection_extension
bb476e2Trac #30208: minor fix

comment:18 Changed 4 months ago by gh-mjungmath

  • Status changed from needs_work to needs_review

I have uploaded the fix of a minor mistake.

comment:19 Changed 4 months ago by git

  • Commit changed from bb476e24da2e55f7133e4d9ff14c92a5792401f9 to a77aebcc00bac6326a4e19a9c67aeb949ee10db2

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

a77aebcTrac #30208: copy_from reverted to recover old behavior

comment:20 Changed 4 months ago by git

  • Commit changed from a77aebcc00bac6326a4e19a9c67aeb949ee10db2 to 934a4398d3659cebb5a376a1369f6168610ddb16

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

934a439documentation slightly improved

comment:21 Changed 4 months ago by tscrim

While this was already there, so I am not going to stop a positive review, but I really dislike doctests like this:

            sage: nab._new_forms(e)  # random
            dict of forms

It is only # random because of the print order, but you can easily make it print in a fixed order by

sage: d = nab._new_forms(e)
sage: [d[k] for k in sorted(d)]

By marking it as # random, it can take any output, so it can mask bugs.

If you don't want to change this, you can just ignore and set a positive review. If you do change, you can just set it to a positive review after you run the doctest on the file.

comment:22 Changed 4 months ago by git

  • Commit changed from 934a4398d3659cebb5a376a1369f6168610ddb16 to d3c8cab917f04ebc9951d41a0cc8dcf4c66392a6

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

d3c8cabTrac #30208: random doctest removed

comment:23 Changed 4 months ago by gh-mjungmath

  • Status changed from needs_review to positive_review

Very good point. Thank you Travis. I've changed it.

comment:24 Changed 4 months ago by gh-mjungmath

I'm just thinking about whether the list assignment via copying is the right way to do. The Python documentation says:

Assignment statements in Python do not copy objects, they create bindings between a target and an object.

I suggest, we remove the __setitem__ method again, and encourage the use of set_connection_form(i, j).copy_from(omega) instead. If that meets with your approval, I will open a ticket for that and apply the same changes for mixed differential forms.

Addendum: Seems it's fine. Even some classes in the Python library admits that behavior. I just improved the documentation regarding that in one of the subsequent tickets.

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

comment:25 Changed 4 months ago by vbraun

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