Opened 7 months ago
Closed 7 months ago
#30209 closed enhancement (fixed)
Action 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: | 7ae2df9 (Commits, GitHub, GitLab) | Commit: | 7ae2df97d0c1d57d8beab4932d5b22ec827f1079 |
Dependencies: | #30208, #30228 | Stopgaps: |
Description (last modified by )
In this ticket, we establish the action of a bundle connection. We implement the component-wise formula based on the connection form.
See Wikipedia for details.
Change History (24)
comment:1 Changed 7 months ago by
- Description modified (diff)
comment:2 Changed 7 months ago by
- Branch set to u/gh-mjungmath/bundle_connection_change_of_frame
comment:3 Changed 7 months ago by
- Commit set to 56996fd684b86009a72ec7aca587ad3e41c7020d
- Description modified (diff)
- Summary changed from Change of Frame Formula for Bundle Connections to Action for Bundle Connections
comment:4 Changed 7 months ago by
- Dependencies changed from #30208 to #30208, #30228
comment:5 Changed 7 months ago by
- Dependencies changed from #30208, #30228 to #30208, #30228, #30239
comment:6 Changed 7 months ago by
- Commit changed from 56996fd684b86009a72ec7aca587ad3e41c7020d to 0e3cba0fc21c4da924bda56aa559ec313eef8376
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
ef8ac4b | Trac #30209: display method + merge
|
51ba4e3 | Trac #30239: comp zero check for __call__
|
bd1f299 | Trac #30209: Merge branch 't/30239/tensorfield___call___alters_zero' into bundle_connection_change_of_frame
|
e7fedb7 | Trac #30209: display method finished
|
b03baa1 | Trac #30209: structure formula fixed
|
bb476e2 | Trac #30208: minor fix
|
a77aebc | Trac #30208: copy_from reverted to recover old behavior
|
934a439 | documentation slightly improved
|
574445d | Trac #30209: Merge branch 'bundle_connection_extension' into bundle_connection_change_of_frame
|
0e3cba0 | Trac #30209: documentation finished + display only_nonzero fixed
|
comment:7 Changed 7 months ago by
- Status changed from new to needs_review
comment:8 Changed 7 months ago by
- Commit changed from 0e3cba0fc21c4da924bda56aa559ec313eef8376 to 2dd78aacdcff90f43f6e0618dbc10b554e7269d7
Branch pushed to git repo; I updated commit sha1. New commits:
2dd78aa | Trac #30209: documentation improved
|
comment:9 Changed 7 months ago by
- Commit changed from 2dd78aacdcff90f43f6e0618dbc10b554e7269d7 to e5a3ac4ebaf740396d1bc32bd7d012d560215318
Branch pushed to git repo; I updated commit sha1. New commits:
e5a3ac4 | Trac #30209: documentation imrpoved
|
comment:10 Changed 7 months ago by
I don't think the depencency of #30239 is necessary anymore. Anyway, I'll leave it this way. Removing it again would be too complicated by now...
comment:11 Changed 7 months ago by
The pycodestyle error has been fixed in #30239.
comment:12 Changed 7 months ago by
- Commit changed from e5a3ac4ebaf740396d1bc32bd7d012d560215318 to 933c14aa016e1cc23be47caf845199c7e1cbf52e
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
504e84e | Trac #30239: Merge branch 't/30266/immutability_for_scalar_fields' into t/30239/tensorfield___call___alters_zero
|
d58d291 | Trac #30266: hash function condition + treatment of restrictions
|
061b89f | Trac #30266: minor doctest improvement
|
5c7fd6e | Trac #30266: immutability of restrictions + hash function improved
|
7ba0865 | Trac #30266: merge
|
0ec99d4 | Trac #30266: check by name
|
87fb411 | Trac #30266: ValueError replaced by AssertionError
|
43ec497 | Trac #30239: referenced before assignment error fixed
|
135ffd5 | Trac #30209: Merge branch 't/30239/tensorfield___call___alters_zero' into bundle_connection_change_of_frame
|
933c14a | Trac #30209: Merge branch 'develop' into bundle_connection_change_of_frame
|
comment:13 Changed 7 months ago by
- Commit changed from 933c14aa016e1cc23be47caf845199c7e1cbf52e to 21f2bd5a70a29dae8cd699ba03f8eb05870bdd7e
comment:14 Changed 7 months ago by
- Dependencies changed from #30208, #30228, #30239 to #30208, #30228
I have removed the dependency of #30239.
comment:15 Changed 7 months ago by
Green patchbot.
comment:16 follow-up: ↓ 17 Changed 7 months ago by
I don't like doctests marked as random that are not random:
sage: nab.connection_forms() # random
Just go through the different values and get the output explicitly (and maybe check the len
). This will be more robust and actually catch errors.
Your error messages in display
don't need to be so excited (just remove the !
from the end).
While it is good to keep encapsulation, just be aware that changes like this:
- rk = vb._rank + rk = vb.rank()
mean extra Python function calls, which are relatively expensive compared to attribute lookup. However, I have no objections to these changes as they seem like they would not have any significant impact on computation times.
comment:17 in reply to: ↑ 16 Changed 7 months ago by
Replying to tscrim:
I don't like doctests marked as random that are not random:
sage: nab.connection_forms() # randomJust go through the different values and get the output explicitly (and maybe check the
len
). This will be more robust and actually catch errors.
Ah, you already mentioned that in some ticket. Sure, I will take care of that.
Your error messages in
display
don't need to be so excited (just remove the!
from the end).
:(
While it is good to keep encapsulation, just be aware that changes like this:
- rk = vb._rank + rk = vb.rank()mean extra Python function calls, which are relatively expensive compared to attribute lookup. However, I have no objections to these changes as they seem like they would not have any significant impact on computation times.
Ah, this is good to know! I'll change it back.
Tomorrow...
comment:18 Changed 7 months ago by
- Commit changed from 21f2bd5a70a29dae8cd699ba03f8eb05870bdd7e to 7ae2df97d0c1d57d8beab4932d5b22ec827f1079
comment:19 Changed 7 months ago by
I see. Something went wrong during the rebase. Please double check in comparison with #30208.
comment:20 Changed 7 months ago by
I've forced a new push btw. Like I said, please take a thorough look.
comment:21 Changed 7 months ago by
Compared with #30208 once again. Looks good, like intended. Patchbot is green again.
comment:22 Changed 7 months ago by
- Reviewers set to Travis Scrimshaw
- Status changed from needs_review to positive_review
Thank you. LGTM.
comment:23 Changed 7 months ago by
Thank you for the review.
comment:24 Changed 7 months ago by
- Branch changed from u/gh-mjungmath/bundle_connection_change_of_frame to 7ae2df97d0c1d57d8beab4932d5b22ec827f1079
- Resolution set to fixed
- Status changed from positive_review to closed
Last 10 new commits:
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
Merge branch 'develop' into bundle_connection_change_of_frame
Trac #30209: forms from scratch
Trac #30209: bundle connection action