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

Action for Bundle Connections #30209

Closed
mjungmath opened this issue Jul 23, 2020 · 33 comments
Closed

Action for Bundle Connections #30209

mjungmath opened this issue Jul 23, 2020 · 33 comments

Comments

@mjungmath
Copy link

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.

Depends on #30208
Depends on #30228

CC: @egourgoulhon @tscrim

Component: manifolds

Author: Michael Jung

Branch/Commit: 7ae2df9

Reviewer: Travis Scrimshaw

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

@mjungmath mjungmath added this to the sage-9.2 milestone Jul 23, 2020
@mjungmath

This comment has been minimized.

@mjungmath
Copy link
Author

@mjungmath
Copy link
Author

Last 10 new commits:

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
b7a6a5fMerge branch 'develop' into bundle_connection_change_of_frame
0c28d89Trac #30209: forms from scratch
56996fdTrac #30209: bundle connection action

@mjungmath
Copy link
Author

Commit: 56996fd

@mjungmath

This comment has been minimized.

@mjungmath mjungmath changed the title Change of Frame Formula for Bundle Connections Action for Bundle Connections Jul 26, 2020
@mjungmath
Copy link
Author

Changed dependencies from #30208 to #30208, #30228

@mjungmath
Copy link
Author

Changed dependencies from #30208, #30228 to #30208, #30228, #30239

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 28, 2020

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

ef8ac4bTrac #30209: display method + merge
51ba4e3Trac #30239: comp zero check for __call__
bd1f299Trac #30209: Merge branch 't/30239/tensorfield___call___alters_zero' into bundle_connection_change_of_frame
e7fedb7Trac #30209: display method finished
b03baa1Trac #30209: structure formula fixed
bb476e2Trac #30208: minor fix
a77aebcTrac #30208: copy_from reverted to recover old behavior
934a439documentation slightly improved
574445dTrac #30209: Merge branch 'bundle_connection_extension' into bundle_connection_change_of_frame
0e3cba0Trac #30209: documentation finished + display only_nonzero fixed

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 28, 2020

Changed commit from 56996fd to 0e3cba0

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 28, 2020

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

2dd78aaTrac #30209: documentation improved

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 28, 2020

Changed commit from 0e3cba0 to 2dd78aa

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 1, 2020

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

e5a3ac4Trac #30209: documentation imrpoved

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 1, 2020

Changed commit from 2dd78aa to e5a3ac4

@mjungmath
Copy link
Author

comment:10

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

@mjungmath
Copy link
Author

comment:11

The pycodestyle error has been fixed in #30239.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 4, 2020

Changed commit from e5a3ac4 to 933c14a

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 4, 2020

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

504e84eTrac #30239: Merge branch 't/30266/immutability_for_scalar_fields' into t/30239/tensorfield___call___alters_zero
d58d291Trac #30266: hash function condition + treatment of restrictions
061b89fTrac #30266: minor doctest improvement
5c7fd6eTrac #30266: immutability of restrictions + hash function improved
7ba0865Trac #30266: merge
0ec99d4Trac #30266: check by name
87fb411Trac #30266: ValueError replaced by AssertionError
43ec497Trac #30239: referenced before assignment error fixed
135ffd5Trac #30209: Merge branch 't/30239/tensorfield___call___alters_zero' into bundle_connection_change_of_frame
933c14aTrac #30209: Merge branch 'develop' into bundle_connection_change_of_frame

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 6, 2020

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

d3c8cabTrac #30208: random doctest removed
5cee80cTrac #30209: Merge branch 'bundle_connection_extension' into bundle_connection_action
21f2bd5Trac #30209: bundle connections acting on sections + vector fields

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 6, 2020

Changed commit from 933c14a to 21f2bd5

@mjungmath
Copy link
Author

comment:14

I have removed the dependency of #30239.

@mjungmath
Copy link
Author

Changed dependencies from #30208, #30228, #30239 to #30208, #30228

@mjungmath
Copy link
Author

comment:15

Green patchbot.

@tscrim
Copy link
Collaborator

tscrim commented Aug 7, 2020

comment:16

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.

@mjungmath
Copy link
Author

comment:17

Replying to @tscrim:

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.

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 8, 2020

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

ec05c7fTrac #30209: Merge branch 't/30208/d3c8cab917f04ebc9951d41a0cc8dcf4c66392a6' into new_bundle_connection_action
7ae2df9Trac #30209: action of bundle connections + display method

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 8, 2020

Changed commit from 21f2bd5 to 7ae2df9

@mjungmath
Copy link
Author

comment:19

I see. Something went wrong during the rebase. Please double check in comparison with #30208.

@mjungmath
Copy link
Author

comment:20

I've forced a new push btw. Like I said, please take a thorough look.

@mjungmath
Copy link
Author

comment:21

Compared with #30208 once again. Looks good, like intended. Patchbot is green again.

@tscrim
Copy link
Collaborator

tscrim commented Aug 9, 2020

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Aug 9, 2020

comment:22

Thank you. LGTM.

@mjungmath
Copy link
Author

comment:23

Thank you for the review.

@vbraun
Copy link
Member

vbraun commented Aug 12, 2020

Changed branch from u/gh-mjungmath/bundle_connection_change_of_frame to 7ae2df9

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

3 participants