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

[SparseArrays] Respect order of mul in (l)mul!(::Diagonal,::Sparse) #30163

Merged
merged 2 commits into from
Dec 13, 2018
Merged

[SparseArrays] Respect order of mul in (l)mul!(::Diagonal,::Sparse) #30163

merged 2 commits into from
Dec 13, 2018

Conversation

dkarrasch
Copy link
Member

@dkarrasch dkarrasch commented Nov 27, 2018

@andreasnoack
Copy link
Member

We actually have

struct Quaternion{T<:Real} <: Number
s::T
v1::T
v2::T
v3::T
end
Quaternion(s::Real, v1::Real, v2::Real, v3::Real) = Quaternion(promote(s, v1, v2, v3)...)
Base.abs2(q::Quaternion) = q.s*q.s + q.v1*q.v1 + q.v2*q.v2 + q.v3*q.v3
Base.abs(q::Quaternion) = sqrt(abs2(q))
Base.real(::Type{Quaternion{T}}) where {T} = T
Base.conj(q::Quaternion) = Quaternion(q.s, -q.v1, -q.v2, -q.v3)
Base.isfinite(q::Quaternion) = isfinite(q.s) & isfinite(q.v1) & isfinite(q.v2) & isfinite(q.v3)
(-)(ql::Quaternion, qr::Quaternion) =
Quaternion(ql.s - qr.s, ql.v1 - qr.v1, ql.v2 - qr.v2, ql.v3 - qr.v3)
(*)(q::Quaternion, w::Quaternion) = Quaternion(q.s*w.s - q.v1*w.v1 - q.v2*w.v2 - q.v3*w.v3,
q.s*w.v1 + q.v1*w.s + q.v2*w.v3 - q.v3*w.v2,
q.s*w.v2 - q.v1*w.v3 + q.v2*w.s + q.v3*w.v1,
q.s*w.v3 + q.v1*w.v2 - q.v2*w.v1 + q.v3*w.s)
(*)(q::Quaternion, r::Real) = Quaternion(q.s*r, q.v1*r, q.v2*r, q.v3*r)
(*)(q::Quaternion, b::Bool) = b * q # remove method ambiguity
(/)(q::Quaternion, w::Quaternion) = q * conj(w) * (1.0 / abs2(w))
(\)(q::Quaternion, w::Quaternion) = conj(q) * w * (1.0 / abs2(q))
exactly for testing this. Maybe we should move the definition to https://github.com/JuliaLang/julia/tree/ca7a692f9f1099ba042ed75e5d83236ec523608a/test/testhelpers where e.g. Furlong already lives such that we can use it here.

@KristofferC KristofferC added sparse Sparse arrays needs tests Unit tests are required for this change bugfix This change fixes an existing bug labels Nov 27, 2018
@dkarrasch
Copy link
Member Author

Ok, cool, I have tests which pass (and wouldn't pass on master for exactly the commutativity issue). It looks like this:

for n in (1, 10)
    v = Quaternion.(rand(n), rand(n), rand(n), rand(n))
    D = Diagonal(v)
    Avals = Quaternion.(rand(2n), rand(2n), rand(2n), rand(2n))
    A = sparse(rand(1:n, 2n), rand(1:n, 2n), Avals, n, n)
    Am = convert(Matrix, A) # test versus dense matrix to make sure different code paths are taken
    @test A * D ≈ Am * D
    @test D * A ≈ D * Am
    @test rmul!(copy(A), D) ≈ Am * D
    @test lmul!(D, copy(A)) ≈ D * Am
end

Comments are welcome. To get this running, I needed to define

Base.zero(::Type{Quaternion{T}}) where T = Quaternion{T}(zero(T), zero(T), zero(T), zero(T))

(+)(ql::Quaternion, qr::Quaternion) =
 Quaternion(ql.s + qr.s, ql.v1 + qr.v1, ql.v2 + qr.v2, ql.v3 + qr.v3)

in addition to what is defined in @andreasnoack 's snippet above. I didn't understand the "move the code" comment above. Should I just add the definitions where the others are and the tests somewhere in the usual test section? Would the tests know about Quaternions?

@KristofferC
Copy link
Member

KristofferC commented Nov 27, 2018

To move the code you can cut and paste the existing Quaternion definition into test/testhelpers/Quaternions.jl. Then you can include that file where it is used in the LinearAlgebra test and also include that file for your sparse multiplication test. You can look at how the other test helpers (like Furlong) is used.

@fredrikekre
Copy link
Member

Could be good to squash this into two commits: one with the move of Quaternion and one with the bugfix, but does not matter much.

@dkarrasch
Copy link
Member Author

Yes, I'll see if I can find a git expert to help me with that.

On a different note: this PR affects #29296, which is labelled as "backport pending 1.0".

@andreasnoack
Copy link
Member

On a different note: this PR affects #29296, which is labelled as "backport pending 1.0".

I think it should be safe to mark this PR for backport as well.

add tests for non-commutative mul
remove quaternions from generic tests
@dkarrasch
Copy link
Member Author

That was the adventure of my life, but I managed to clean up my commit mess (with the help of my colleague... well, he dictated, I was typing). Result: two beautiful, nicely sorted commits. 🎉

@KristofferC KristofferC mentioned this pull request Nov 28, 2018
61 tasks
@chriscoey
Copy link

can this be merged?

@ViralBShah
Copy link
Member

I believe so - but I am wondering if we need to wait for the 1.1 release process.

@dkarrasch
Copy link
Member Author

can this be merged?

Yes. I wasn't sure if I needed to give another "I'm done" signal, since I felt that was obvious and didn't want to put pressure on anyone.

@andreasnoack andreasnoack merged commit 3ee8798 into JuliaLang:master Dec 13, 2018
KristofferC pushed a commit that referenced this pull request Dec 30, 2018
…30163)

* order of mul in (l)mul!(::Diagonal,::Sparse)

add tests for non-commutative mul

* Create Quaternions.jl

remove quaternions from generic tests

(cherry picked from commit 3ee8798)
@KristofferC KristofferC mentioned this pull request Dec 30, 2018
53 tasks
@StefanKarpinski StefanKarpinski added triage This should be discussed on a triage call backport 1.0 and removed triage This should be discussed on a triage call labels Jan 31, 2019
@KristofferC
Copy link
Member

Can change behavior so should not be backported

@KristofferC KristofferC removed backport 1.0 triage This should be discussed on a triage call labels Jan 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug needs tests Unit tests are required for this change sparse Sparse arrays
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mul! of sparse and diagonal for non-commutative eltypes?
7 participants