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

[Merged by Bors] - feat(linear_algebra/{cross_product,vectors}): cross product #11181

Closed
wants to merge 40 commits into from

Conversation

madvorak
Copy link
Collaborator

@madvorak madvorak commented Jan 1, 2022

Defines the cross product for R^3 and gives localized notation for that and the dot product.

Was #10959

Co-authored-by: Kyle Miller kmill31415@gmail.com
Co-authored-by: Eric Wieser wieser.eric@gmail.com


Open in Gitpod

Copy link
Collaborator

@kmill kmill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

@kmill kmill added the awaiting-review The author would like community review of the PR label Jan 2, 2022
@kmill kmill changed the title feat(linear_algebra/cross_product) feat(linear_algebra/{cross_product,vectors}): cross product Jan 2, 2022
Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth including a few statements about u ×₃ v ×₃ w, or do you intend to leave that to a future PR? If so, can you add a TODO section in the module docstring, which should probably contains

  • The Jacobi identity
  • Generalization to an arbitrary 3D inner product space - I know we decided on Zulip it wasn't worth doing this, but we should record that decision in the docstring somewhere.

@madvorak madvorak removed the awaiting-review The author would like community review of the PR label Jan 3, 2022
madvorak and others added 3 commits January 3, 2022 11:21
docstring comment for triple_product_eq_det

Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
@madvorak
Copy link
Collaborator Author

madvorak commented Jan 3, 2022

  • The Jacobi identity

I have just implemented it. Please, check the name and the docstring.

  • Generalization to an arbitrary 3D inner product space - I know we decided on Zulip it wasn't worth doing this, but we should record that decision in the docstring somewhere.

How should I put it into the docstring?
I suppose it should be in the main docstring for the file but I don't know how to formulate it. And should a link to the Zulip discussion be included?

@eric-wieser eric-wieser added the awaiting-author A reviewer has asked the author a question or requested changes label Jan 4, 2022
@madvorak madvorak added blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. and removed awaiting-author A reviewer has asked the author a question or requested changes labels Jan 4, 2022
@github-actions github-actions bot removed the blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. label Jan 4, 2022
Copy link
Member

@jcommelin jcommelin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 🎉

bors merge

bors bot pushed a commit that referenced this pull request Jan 13, 2022
Defines the cross product for R^3 and gives localized notation for that and the dot product.

Was #10959

Co-authored-by: Kyle Miller <kmill31415@gmail.com>
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>



Co-authored-by: Martin Dvořák <martin.dvorak@matfyz.cz>
@bors
Copy link

bors bot commented Jan 13, 2022

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Jan 13, 2022
Defines the cross product for R^3 and gives localized notation for that and the dot product.

Was #10959

Co-authored-by: Kyle Miller <kmill31415@gmail.com>
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>



Co-authored-by: Martin Dvořák <martin.dvorak@matfyz.cz>
@madvorak
Copy link
Collaborator Author

bors r-

Does 100 characters and a newline count as >100 lines?

@bors
Copy link

bors bot commented Jan 13, 2022

Canceled.

@madvorak
Copy link
Collaborator Author

lemme try whether it allows me to rerun it

bors merge

@bors
Copy link

bors bot commented Jan 13, 2022

🔒 Permission denied

Existing reviewers: click here to make madvorak a reviewer

@jcommelin
Copy link
Member

Thanks 🎉

If CI passes, please remove the label awaiting-CI and merge this yourself, by adding a comment bors r+.

bors d+

@bors
Copy link

bors bot commented Jan 13, 2022

✌️ madvorak can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@github-actions github-actions bot added the delegated The PR author may merge after reviewing final suggestions. label Jan 13, 2022
@eric-wieser
Copy link
Member

The long line wasn't in this PR, it was in #10269. By cancelling, you've just made things slower, bors already worked this out for you!

@madvorak
Copy link
Collaborator Author

bors r+

@erdOne
Copy link
Member

erdOne commented Jan 13, 2022

Its me :(
sorry about that confusion.

@madvorak
Copy link
Collaborator Author

madvorak commented Jan 13, 2022

Its me :( sorry about that confusion.

No worries.

BTW did my previous comment resume the merging?

@madvorak
Copy link
Collaborator Author

bors merge

@bors
Copy link

bors bot commented Jan 13, 2022

Already running a review

bors bot pushed a commit that referenced this pull request Jan 13, 2022
Defines the cross product for R^3 and gives localized notation for that and the dot product.

Was #10959

Co-authored-by: Kyle Miller <kmill31415@gmail.com>
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>



Co-authored-by: Martin Dvořák <martin.dvorak@matfyz.cz>
@bors
Copy link

bors bot commented Jan 13, 2022

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(linear_algebra/{cross_product,vectors}): cross product [Merged by Bors] - feat(linear_algebra/{cross_product,vectors}): cross product Jan 13, 2022
@bors bors bot closed this Jan 13, 2022
@bors bors bot deleted the madvorak_cross_product branch January 13, 2022 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
delegated The PR author may merge after reviewing final suggestions. ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants