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

Add matrix exponential for 1x1 and 2x2, and fallback to Base (fixes #159) #160

Merged
merged 8 commits into from
May 23, 2017

Conversation

Godisemo
Copy link
Contributor

@Godisemo Godisemo commented May 10, 2017

I have only tried this in julia v0.5, which is why the PR is to the julia-0.5 branch. You could probably cherry pick it to master.

Note: On my computer this gives a speedup of over 300x

a = randn(SMatrix{2,2})
b = Array(a)

@benchmark expm(a)
# BenchmarkTools.Trial:
#  memory estimate:  48 bytes
#  allocs estimate:  1
#  --------------
#  minimum time:     57.861 ns (0.00% GC)
#  median time:      58.580 ns (0.00% GC)
#  mean time:        65.692 ns (8.97% GC)
#  maximum time:     3.145 μs (96.47% GC)
#  --------------
#  samples:          10000
#  evals/sample:     983
@benchmark expm(b)
# BenchmarkTools.Trial:
#  memory estimate:  5.67 KiB
#  allocs estimate:  76
#  --------------
#  minimum time:     15.294 μs (0.00% GC)
#  median time:      18.114 μs (0.00% GC)
#  mean time:        19.030 μs (4.38% GC)
#  maximum time:     4.279 ms (98.87% GC)
#  --------------
#  samples:          10000
#  evals/sample:     1

@Godisemo Godisemo mentioned this pull request May 10, 2017
@Godisemo Godisemo changed the title Add matrix exponential for 1x1 and 2x2, and fallback to Base (issue #159) Add matrix exponential for 1x1 and 2x2, and fallback to Base (fixes #159) May 10, 2017
Copy link
Member

@andyferris andyferris left a comment

Choose a reason for hiding this comment

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

Thanks @Godisemo! This looks nice, and is appreciated.

I think that the functionality here is OK to merge (there are some necessary fixes in the code comments).

In the future it would be nice to leverage eig or other decompositions to do this... e.g. since we can do eig for symmetric/hermitian 3x3 matrices then we extend expm for symmetric/hermitian 3x3 matrices. (Arguably we could define all the functions which work on in independent eigenvalues like expm, logm, etc in terms of eig, and add specializations to that over time). To be clear, I don't think this is necessary to be part of this PR, though.

src/expm.jl Outdated
@inline expm(A::StaticMatrix) = _expm(Size(A), A)

@generated function _expm(::Size{(1,1)}, A::StaticMatrix)
@assert size(A) == (1,1)
Copy link
Member

Choose a reason for hiding this comment

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

Probably overkill, given the above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean the assertion?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah

src/expm.jl Outdated

@generated function _expm(::Size{(1,1)}, A::StaticMatrix)
@assert size(A) == (1,1)
T = promote_type(typeof(sqrt(one(eltype(A)))), Float32)
Copy link
Member

Choose a reason for hiding this comment

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

In Julia v0.6, you can only perform "hyper"-pure operations inside the function generator. Since the eltype might not be defined before static arrays is loaded, this just isn't "pure enough" for the assumptions made by the Julia 0.6 compiler.

However, it's safe to move this into the body of the function (inside the quote) - where possible inference will perform type calculations at compile time.

Also - why sqrt(one) not exp(zero)? (zero is slightly more flexible from a group vs. algebra perspective, i.e. in theory eltype(A) might be a generator that supports addition and zero only.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving

        T = promote_type(typeof(exp(zero(eltype(A)))), Float32)
        newtype = similar_type(A,T)

into the body removes the need for using a generated functions in the first place, right?

Copy link
Member

Choose a reason for hiding this comment

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

actually, yes! I think the eig and inv code works like that.

src/expm.jl Outdated
@generated function _expm(::Size{(1,1)}, A::StaticMatrix)
@assert size(A) == (1,1)
T = promote_type(typeof(sqrt(one(eltype(A)))), Float32)
newtype = similar_type(A,T)
Copy link
Member

Choose a reason for hiding this comment

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

Also move this to body - it's "free"

src/expm.jl Outdated
@generated function _expm{S<:Real}(::Size{(2,2)}, A::StaticMatrix{S})
@assert size(A) == (2,2)
T = promote_type(typeof(sqrt(one(eltype(A)))), Float32)
newtype = similar_type(A,T)
Copy link
Member

Choose a reason for hiding this comment

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

Move these two lines to the body.

src/expm.jl Outdated
v = (a-d)^2 + 4*b*c

# if v == 0
z1::$T = 1.0
Copy link
Member

Choose a reason for hiding this comment

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

These are unnecessary since if statements don't introduce a new scoping block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for this was to ensure type stability. Moved it to the else-block instead.

src/expm.jl Outdated

@generated function _expm(::Size, A::StaticArray)
T = promote_type(typeof(sqrt(one(eltype(A)))), Float32)
newtype = similar_type(A,T)
Copy link
Member

Choose a reason for hiding this comment

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

Move these two lines to the generated body

src/expm.jl Outdated

quote
$(Expr(:meta, :inline))
($newtype)(Base.expm(Array(A)))
Copy link
Member

Choose a reason for hiding this comment

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

Generally, I've been using SizedArray for this purpose with eig, inv, etc. The static size of the array is not forgotten, the data doesn't need to be copied to the stack unnecessarily, and the user gets feedback that a fully "native" static arrays method was not used.

However, it does violate similar_type, which doesn't seem that desirable...

src/expm.jl Outdated
@inline expm(A::StaticMatrix) = _expm(Size(A), A)

@inline function _expm(::Size{(1,1)}, A::StaticMatrix)
T = promote_type(typeof(exp(zero(eltype(A)))), Float32)
Copy link
Member

Choose a reason for hiding this comment

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

Why are we promoting with Float32 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I probably copied from the cholesky source

src/expm.jl Outdated

@inline function _expm(s::Size, A::StaticArray)
T = promote_type(typeof(exp(zero(eltype(A)))), Float32)
newtype = similar_type(A,T)
Copy link
Member

Choose a reason for hiding this comment

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

newtype unused

z1 = cos(z / 2)
z2 = sin(z / 2) / z
else # if v == 0
z1 = T(1.0)
Copy link
Member

@andyferris andyferris May 11, 2017

Choose a reason for hiding this comment

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

zero(T) one(T)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

one(T)?

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe not. I dunno.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since there is no half(T), I would prefer not to use one(T)

(newtype)((exp(A[1]), ))
end

@inline function _expm{S<:Real}(::Size{(2,2)}, A::StaticMatrix{S})
Copy link
Member

Choose a reason for hiding this comment

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

StaticMatrix has new type parameters, StaticMatrix{M,N,T}.

Try _expm(::Size{(2,2)}, A::StaticMatrix{<:Any, <:Any, <:Real})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then it won't work on 0.5 though. How to handle backwards compatibility?

Copy link
Member

Choose a reason for hiding this comment

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

How to handle backwards compatibility?

We don't :) They have diverged.

It's not an ideal situation, but that's where we are.

Copy link
Member

Choose a reason for hiding this comment

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

I think there is a branch somewhere where we can merge backports. It's just more work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, up until the last commit, it worked on julia 0.5. I think I might have messed up when changing target branch for this PR to master.

Copy link
Member

Choose a reason for hiding this comment

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

I see... there seems to be some conflicts.

One way of doing this one PR to julia-0.5 and one PR to master. The difference will be trivial.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I changed back to the julia-0.5 branch. Will copy the files over to master and create a new PR. You will have to revert the last commit though and pull 1f2ff2b

@Godisemo Godisemo changed the base branch from julia-0.5 to master May 11, 2017 01:08
@Godisemo Godisemo changed the base branch from master to julia-0.5 May 11, 2017 01:15
@Godisemo
Copy link
Contributor Author

Is this PR and #161 not ready for merging?

@andyferris
Copy link
Member

Travis is showing errors...

@Godisemo
Copy link
Contributor Author

Godisemo commented May 23, 2017

You can clearly see that the errors are caused by previous commits... The tests that fail are related to Binary IO and write. As for #161 I don't see how that error is related to these changes either.

@andyferris
Copy link
Member

I'm totally confused.

I re-ran travis... what exactly are these errors about?

@c42f
Copy link
Member

c42f commented May 23, 2017

Yes, these errors are really weird. Looks like I forgot a Compat dependency for take! when doing the IO tests... but as to why the tests pass locally I'm really confused.

@c42f c42f mentioned this pull request May 23, 2017
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 72fff23 on Godisemo:matrix-exponential into ** on JuliaArrays:julia-0.5**.

@c42f
Copy link
Member

c42f commented May 23, 2017

After merging #174 the test failures here are now only spurious failures on nightly - tests for julia-0.5 pass so I'm going to merge this.

@c42f c42f merged commit c353641 into JuliaArrays:julia-0.5 May 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants