Opened 9 months ago
Closed 8 months ago
#30208 closed enhancement (fixed)
List Assignment for Bundle Connections
Reported by:  ghmjungmath  Owned by:  

Priority:  major  Milestone:  sage9.2 
Component:  manifolds  Keywords:  
Cc:  egourgoulhon, tscrim  Merged in:  
Authors:  Michael Jung  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  d3c8cab (Commits, GitHub, GitLab)  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 1form 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 9 months ago by
 Dependencies set to #30191
comment:2 Changed 9 months ago by
 Branch set to u/ghmjungmath/bundle_connection_extension
comment:3 Changed 9 months ago by
 Commit set to fa87f4b3f439fd02cbef4622efcebb11da329d5f
 Status changed from new to needs_review
comment:4 Changed 9 months ago by
 Commit changed from fa87f4b3f439fd02cbef4622efcebb11da329d5f to 4f0c279585c38920b442426c081bf93ecc526034
Branch pushed to git repo; I updated commit sha1. New commits:
4f0c279  Trac #30208: Merge branch 'develop' into bundle_connection_extension

comment:5 Changed 9 months ago by
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 9 months ago by
 Commit changed from 4f0c279585c38920b442426c081bf93ecc526034 to 41983daf47bc0ab84485cc19a0b25f582465c343
Branch pushed to git repo; I updated commit sha1. New commits:
41983da  Trac #30208: form input deprecated

comment:7 Changed 9 months ago by
Is this suitable?
Addendum: Do you think this attempt is better than the one before?
comment:8 followup: ↓ 9 Changed 9 months ago by
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 ; followup: ↓ 10 Changed 9 months ago by
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 ; followup: ↓ 11 Changed 9 months ago by
Replying to ghmjungmath:
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 9 months ago by
comment:12 Changed 9 months ago by
 Commit changed from 41983daf47bc0ab84485cc19a0b25f582465c343 to f9cb6c14c1709302fdbf2d37bae69a0ae098ebe6
Branch pushed to git repo; I updated commit sha1. New commits:
f9cb6c1  Trac #30208: deprecation warning

comment:13 Changed 9 months ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
LGTM.
comment:14 Changed 9 months ago by
Thank you for the positive review. Besides, I am still curious what Eric has to say about these changes.
comment:15 Changed 9 months ago by
 Status changed from positive_review to needs_work
Sorry, I have found some severe flaws.
comment:16 Changed 9 months ago by
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, yz, 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 3dimensional 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 3dimensional 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.
comment:17 Changed 9 months ago by
 Commit changed from f9cb6c14c1709302fdbf2d37bae69a0ae098ebe6 to bb476e24da2e55f7133e4d9ff14c92a5792401f9
Branch pushed to git repo; I updated commit sha1. New commits:
c34e481  Trac #30191: remove in ZZ check

65dff3d  Trac #30191: Merge branch 'develop' into conversion_attributeerror_fix

acc407c  Trac #30191: tryexcept block for zero check

0d27b57  Trac #30191: unnecessary comment block removed

f94bcbe  Trac #30191: if blocks rearranged

28dec69  Trac #30191: doctest errors fixed

6a55a0b  Merge branch 'conversion_attributeerror_fix' into bundle_connection_extension

bb476e2  Trac #30208: minor fix

comment:18 Changed 9 months ago by
 Status changed from needs_work to needs_review
I have uploaded the fix of a minor mistake.
comment:19 Changed 9 months ago by
 Commit changed from bb476e24da2e55f7133e4d9ff14c92a5792401f9 to a77aebcc00bac6326a4e19a9c67aeb949ee10db2
Branch pushed to git repo; I updated commit sha1. New commits:
a77aebc  Trac #30208: copy_from reverted to recover old behavior

comment:20 Changed 9 months ago by
 Commit changed from a77aebcc00bac6326a4e19a9c67aeb949ee10db2 to 934a4398d3659cebb5a376a1369f6168610ddb16
Branch pushed to git repo; I updated commit sha1. New commits:
934a439  documentation slightly improved

comment:21 Changed 9 months ago by
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 9 months ago by
 Commit changed from 934a4398d3659cebb5a376a1369f6168610ddb16 to d3c8cab917f04ebc9951d41a0cc8dcf4c66392a6
Branch pushed to git repo; I updated commit sha1. New commits:
d3c8cab  Trac #30208: random doctest removed

comment:23 Changed 9 months ago by
 Status changed from needs_review to positive_review
Very good point. Thank you Travis. I've changed it.
comment:24 Changed 9 months ago by
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.
comment:25 Changed 8 months ago by
 Branch changed from u/ghmjungmath/bundle_connection_extension to d3c8cab917f04ebc9951d41a0cc8dcf4c66392a6
 Resolution set to fixed
 Status changed from positive_review to closed
Last 10 new commits:
Trac #30191: code improvements + in ZZ check removed again
Trac #30191: != [] removal reverted
Trac #30191: elif reverted + in ZZ check for automorphismfield_group
Trac #30191: minor fix
Trac #30191: minor fix
Trac# 30191: wrong indentation reverted
Trac #30191: comp check unified to if comp
Trac #30191: unused ScalarField import removed
Trac #30208: assigment for bundle connections
Trac #30208: documentation improved