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

LinearAlgebra support for cross product of 3-tuples #56259

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MilesCranmer
Copy link
Member

@MilesCranmer MilesCranmer commented Oct 21, 2024

The current behavior is:

julia> using LinearAlgebra

julia> norm((1, 2, 3))
3.7416573867739413

julia> cross((1, 2, 3), (4, 5, 6))
ERROR: MethodError: no method matching cross(::Tuple{Int64, Int64, Int64}, ::Tuple{Int64, Int64, Int64})
The function `cross` exists, but no method is defined for this combination of argument types.

Closest candidates are:
  cross(::AbstractVector, ::AbstractVector)

with this PR:

julia> cross((1, 2, 3), (4, 5, 6))
(3, -6, 3)

The PR is pretty minimal, basically all I did was move the current code to the 3-tuple version, and then wrap it with a collect in the vector version.

@giordano giordano added the linear algebra Linear algebra label Oct 21, 2024
@LilithHafner LilithHafner added performance Must go faster feature Indicates new feature / enhancement requests needs tests Unit tests are required for this change labels Oct 22, 2024
@LilithHafner
Copy link
Member

We should definitely accept tuples if the length is known at design time (which it is). This should have tests. Tagging triage because it's a new API

@LilithHafner LilithHafner added the triage This should be discussed on a triage call label Oct 22, 2024
@MilesCranmer
Copy link
Member Author

I added the test to the jldoctest. Lmk if that's good enough or not. The code for the vector unittests will now hit the tuple version though so should be tested elsewhere anyways.

@LilithHafner
Copy link
Member

Sounds good enough to me.

@LilithHafner LilithHafner removed the needs tests Unit tests are required for this change label Oct 23, 2024
@fredrikekre
Copy link
Member

These types of PRs are usually rejected with a pointer to StaticArrays.jl.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Indicates new feature / enhancement requests linear algebra Linear algebra performance Must go faster triage This should be discussed on a triage call
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants