Opened 2 years ago
Closed 23 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, 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 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 2 years ago by
- Dependencies set to #30191
comment:2 Changed 2 years ago by
- Branch set to u/gh-mjungmath/bundle_connection_extension
comment:3 Changed 2 years ago by
- Commit set to fa87f4b3f439fd02cbef4622efcebb11da329d5f
- Status changed from new to needs_review
comment:4 Changed 2 years 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 2 years 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 2 years 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 2 years ago by
Is this suitable?
Addendum: Do you think this attempt is better than the one before?
comment:8 follow-up: ↓ 9 Changed 2 years 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 ; follow-up: ↓ 10 Changed 2 years 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 ; follow-up: ↓ 11 Changed 2 years ago by
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 2 years ago by
comment:12 Changed 2 years 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 2 years ago by
- Reviewers set to Travis Scrimshaw
- Status changed from needs_review to positive_review
LGTM.
comment:14 Changed 2 years ago by
Thank you for the positive review. Besides, I am still curious what Eric has to say about these changes.
comment:15 Changed 2 years ago by
- Status changed from positive_review to needs_work
Sorry, I have found some severe flaws.
comment:16 Changed 2 years 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, 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.
comment:17 Changed 2 years 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: try-except 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 2 years ago by
- Status changed from needs_work to needs_review
I have uploaded the fix of a minor mistake.
comment:19 Changed 2 years 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 2 years 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 2 years 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 2 years 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 2 years ago by
- Status changed from needs_review to positive_review
Very good point. Thank you Travis. I've changed it.
comment:24 Changed 2 years 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.
comment:25 Changed 23 months ago by
- Branch changed from u/gh-mjungmath/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