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

Support Val{k} as second argument in diagm #255

Merged
merged 2 commits into from
Jul 27, 2017

Conversation

wsshin
Copy link
Contributor

@wsshin wsshin commented Jul 26, 2017

This PR implements diagm(::SVector, ::Type{Val{k}}), which type-stably creates a matrix with a specified kth super-or sub-diagonal. Previously there was only diagm(::SVector) without the second argument.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 89.029% when pulling b3c5130 on wsshin:diagm-with-Val into 0a73072 on JuliaArrays:master.

@codecov-io
Copy link

codecov-io commented Jul 26, 2017

Codecov Report

Merging #255 into master will decrease coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #255      +/-   ##
==========================================
- Coverage      89%   88.95%   -0.06%     
==========================================
  Files          36       36              
  Lines        2619     2625       +6     
==========================================
+ Hits         2331     2335       +4     
- Misses        288      290       +2
Impacted Files Coverage Δ
src/linalg.jl 97.26% <100%> (+0.09%) ⬆️
src/eigen.jl 91.32% <0%> (-0.92%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a73072...43b8bfa. Read the comment docs.

@wsshin wsshin closed this Jul 26, 2017
@wsshin wsshin reopened this Jul 26, 2017
@wsshin wsshin closed this Jul 26, 2017
@wsshin wsshin reopened this Jul 26, 2017
@wsshin wsshin closed this Jul 26, 2017
@wsshin wsshin reopened this Jul 26, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 89.029% when pulling b3c5130 on wsshin:diagm-with-Val into 0a73072 on JuliaArrays:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 88.952% when pulling b3c5130 on wsshin:diagm-with-Val into 0a73072 on JuliaArrays:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 88.952% when pulling b3c5130 on wsshin:diagm-with-Val into 0a73072 on JuliaArrays:master.

Copy link
Member

@c42f c42f left a comment

Choose a reason for hiding this comment

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

Looks nice to me, thanks!

T = eltype(v)
exprs = [i == j ? :(v[$i]) : zero(T) for i = 1:S[1], j = 1:S[1]]
ind = diagind(Snew1, Snew1, D)
Copy link
Member

Choose a reason for hiding this comment

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

Nice. I didn't know about diagind.

test/linalg.jl Outdated
@@ -40,6 +40,8 @@ using StaticArrays, Base.Test

@testset "diagm()" begin
@test @inferred(diagm(SVector(1,2))) === @SMatrix [1 0; 0 2]
@test @inferred(diagm(SVector(1,2,3), Val{2})) == diagm([1,2,3], 2)
Copy link
Member

Choose a reason for hiding this comment

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

A minor thing - might be worth adding a type assert here ::SMatrix on the left hand side outside the @inferred to assert that these come out as static arrays.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Just made changes accordingly.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 88.952% when pulling 43b8bfa on wsshin:diagm-with-Val into 0a73072 on JuliaArrays:master.

@c42f c42f merged commit da1a371 into JuliaArrays:master Jul 27, 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.

4 participants