Porting SDiagonal from Bridge.jl #240

mschauer · 2017-07-02T08:20:25Z

This was originally written for FixedSizeArrays and ported to StaticArrays by @getzdan. As I think we both still aren't very familiar with the internals of StaticArrays please review carefully.

Tag #235

Originally contributed by D. Getz (https://github.com/getzdan), M. Schauer at https://github.com/mschauer/Bridge.jl under MIT License

mschauer · 2017-07-02T08:29:32Z

I add tests the following days if nobody beats me.

coveralls · 2017-07-02T08:41:27Z

Coverage decreased (-2.5%) to 85.658% when pulling aebf3bc on mschauer:SDiagonal into 80b6bac on JuliaArrays:master.

codecov-io · 2017-07-02T08:41:44Z

Codecov Report

Merging #240 into master will decrease coverage by 0.3%.
The diff coverage is 83.05%.

@@            Coverage Diff             @@
##           master     #240      +/-   ##
==========================================
- Coverage   88.11%   87.81%   -0.31%     
==========================================
  Files          33       34       +1     
  Lines        2247     2306      +59     
==========================================
+ Hits         1980     2025      +45     
- Misses        267      281      +14

Impacted Files	Coverage Δ
src/StaticArrays.jl	`100% <ø> (ø)`	⬆️
src/SDiagonal.jl	`83.05% <83.05%> (ø)`
src/eigen.jl	`90.41% <0%> (-1.83%)`	⬇️

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 4e1a70b...38c9311. Read the comment docs.

coveralls · 2017-07-02T13:51:06Z

Coverage decreased (-2.5%) to 85.633% when pulling 4be7637 on mschauer:SDiagonal into 80b6bac on JuliaArrays:master.

coveralls · 2017-07-02T14:17:48Z

Coverage decreased (-2.3%) to 85.82% when pulling 1eeb98b on mschauer:SDiagonal into 80b6bac on JuliaArrays:master.

coveralls · 2017-07-02T19:32:20Z

Coverage decreased (-1.9%) to 86.263% when pulling 57f2526 on mschauer:SDiagonal into 80b6bac on JuliaArrays:master.

coveralls · 2017-07-02T19:43:34Z

Coverage decreased (-1.8%) to 86.35% when pulling 57f2526 on mschauer:SDiagonal into 80b6bac on JuliaArrays:master.

mschauer · 2017-07-02T21:02:17Z

I put some more work into this, this is probably halfway done, help is most welcome.

coveralls · 2017-07-02T21:07:57Z

Coverage decreased (-1.1%) to 86.977% when pulling 0772fb6 on mschauer:SDiagonal into 80b6bac on JuliaArrays:master.

andyferris

Thanks for all of this - appreciated very much.

Don't be scared by the number of comments... they are each pretty simple. :)

andyferris · 2017-07-03T03:53:55Z

src/SDiagonal.jl

+
+Base.@propagate_inbounds function getindex{N,T}(D::SDiagonal{N,T}, i::Int, j::Int)  
+    @boundscheck checkbounds(D, i, j)
+    if i == j


This is a perfect place to use ifelse instead of branching (will produce faster code)

also looks nicer

andyferris · 2017-07-03T04:03:49Z

src/SDiagonal.jl

+    end
+end
+
+# avoid linear indexing?


Would be nice. Unfortunately most the static arrays internal code assumes a linear indexing style - we should change this, but it won't be easy...

andyferris · 2017-07-03T04:04:50Z

src/SDiagonal.jl

+convert{N,T}(::Type{SDiagonal{N,T}}, D::SDiagonal{N,T}) = D
+convert{N,T}(::Type{SDiagonal{N,T}}, D::SDiagonal) = SDiagonal{N,T}(convert(SVector{N,T}, D.diag))
+
+size{N}(D::SDiagonal{N}) = (N,N)


Is this necessary? There should be a fallback already.

andyferris · 2017-07-03T04:05:09Z

src/SDiagonal.jl

+
+
+convert{N,T}(::Type{SDiagonal{N,T}}, D::SDiagonal{N,T}) = D
+convert{N,T}(::Type{SDiagonal{N,T}}, D::SDiagonal) = SDiagonal{N,T}(convert(SVector{N,T}, D.diag))


D::SDiagonal{N} maybe?

andyferris · 2017-07-03T04:05:45Z

src/SDiagonal.jl

+# Originally contributed by D. Getz (https://github.com/getzdan), M. Schauer
+# at https://github.com/mschauer/Bridge.jl under MIT License
+
+import Base: getindex,setindex!,==,-,+,*,/,\,transpose,ctranspose,convert, size, abs, real, imag, conj, eye, inv


merge these imports with the global ones?

andyferris · 2017-07-03T04:13:24Z

src/SDiagonal.jl

+/{T<:Number}(D::SDiagonal, x::T) = SDiagonal(D.diag / x)
+*(Da::SDiagonal, Db::SDiagonal) = SDiagonal(Da.diag .* Db.diag)
+*(D::SDiagonal, V::SVector) = D.diag .* V
+*(V::SVector, D::SDiagonal) = D.diag .* V


Should V be a RowVector?

or rather: this is already a fallback https://github.com/JuliaLang/julia/blob/master/base/linalg/rowvector.jl#L181 which looks like it is no overhead

andyferris · 2017-07-03T04:13:49Z

src/SDiagonal.jl

+*(Da::SDiagonal, Db::SDiagonal) = SDiagonal(Da.diag .* Db.diag)
+*(D::SDiagonal, V::SVector) = D.diag .* V
+*(V::SVector, D::SDiagonal) = D.diag .* V
+*(A::SMatrix, D::SDiagonal) = scalem(A,D.diag)


A::AbstractMatrix (and below)

andyferris · 2017-07-03T04:15:09Z

src/SDiagonal.jl

+*(V::SVector, D::SDiagonal) = D.diag .* V
+*(A::SMatrix, D::SDiagonal) = scalem(A,D.diag)
+*(D::SDiagonal, A::SMatrix) = scalem(D.diag,A)
+\(D::SDiagonal, b::SVector) = D.diag .\ b


Can we make this work for matrices as well?

also /(vector, diagonal)?

andyferris · 2017-07-03T04:15:59Z

src/SDiagonal.jl

+logm(D::SDiagonal) = SDiagonal(log.(D.diag))
+sqrtm(D::SDiagonal) = SDiagonal(sqrt.(D.diag))
+
+\(D::SDiagonal, B::SMatrix) = scalem(1 ./ D.diag, B)


B::AbstractMatrix (and below)

andyferris · 2017-07-03T04:16:53Z

src/SDiagonal.jl

+/(Da::SDiagonal, Db::SDiagonal) = SDiagonal(Da.diag ./ Db.diag )
+
+function inv{N,T}(D::SDiagonal{N,T})
+    for i = 1:N


Possibly @inbounds ?

Also, we generally unroll such loops elsewhere...

It's also slightly annoying that the code to check if we should throw a singular exception will be slower than taking the inverse itself... but I guess Base.Diagonal does this too?

Yes, but I guess the divisions will still be the most expensive operation here.

mschauer · 2017-07-03T10:41:44Z

Not at all, thank you for the careful reading!

coveralls · 2017-07-03T10:52:25Z

Coverage decreased (-1.09%) to 87.042% when pulling 1244a2c on mschauer:SDiagonal into 80b6bac on JuliaArrays:master.

coveralls · 2017-07-03T11:05:29Z

Coverage decreased (-1.2%) to 86.917% when pulling 1244a2c on mschauer:SDiagonal into 80b6bac on JuliaArrays:master.

andyferris · 2017-07-04T04:51:51Z

src/SDiagonal.jl

+
+@generated function scalem{M, N}(a::StaticMatrix{M,N}, b::StaticVector{N})
+    expr = vec([:(a[$j,$i]*b[$i]) for j=1:M, i=1:N])
+    :(let val1 = ($(expr[1])); similar_type(SMatrix{M,N},typeof(val1))(val1, $(expr[2:end]...)); end)


Perhaps these should be inlined?

:(@_inline_meta; let ... )

andyferris · 2017-07-04T04:55:37Z

src/SDiagonal.jl

+convert{N,T}(::Type{SDiagonal{N,T}}, D::SDiagonal{N,T}) = D
+convert{N,T}(::Type{SDiagonal{N,T}}, D::SDiagonal{N}) = SDiagonal{N,T}(convert(SVector{N,T}, D.diag))
+
+Base.@propagate_inbounds function getindex{N,T}(D::SDiagonal{N,T}, i::Int, j::Int)  


@propagate_inbounds is needed if you want to defer bounds checking to an inner function. This should be simply @inline since you are doing your own @boundscheck and @inbounds here.

andyferris · 2017-07-04T04:58:03Z

A final style note is that we've been moving to using where syntax everywhere, e.g instead of f{T}(x::T) = ... we use f(x::T) where {T} = ....

andyferris · 2017-07-04T05:00:06Z

src/SDiagonal.jl

+end
+
+# avoid linear indexing?
+Base.@propagate_inbounds function getindex{N,T}(D::SDiagonal{N,T}, k::Int) 


This is a good use of @propagate_inbounds.

However, it has already been imported so you don't need the Base.

coveralls · 2017-07-04T07:04:04Z

Coverage decreased (-1.05%) to 87.077% when pulling 8ff246c on mschauer:SDiagonal into 80b6bac on JuliaArrays:master.

coveralls · 2017-07-04T07:12:50Z

Coverage decreased (-1.04%) to 87.077% when pulling ec68f06 on mschauer:SDiagonal into 4e1a70b on JuliaArrays:master.

coveralls · 2017-07-04T22:29:51Z

Coverage decreased (-0.3%) to 87.814% when pulling 4f943ef on mschauer:SDiagonal into 4e1a70b on JuliaArrays:master.

coveralls · 2017-07-04T22:41:44Z

Coverage decreased (-0.4%) to 87.728% when pulling 38c9311 on mschauer:SDiagonal into 4e1a70b on JuliaArrays:master.

mschauer · 2017-07-04T22:47:41Z

@andyferris I hope I addressed all your comments.

coveralls · 2017-07-04T22:53:02Z

Coverage decreased (-0.3%) to 87.814% when pulling 38c9311 on mschauer:SDiagonal into 4e1a70b on JuliaArrays:master.

coveralls · 2017-07-04T23:03:41Z

Coverage decreased (-0.3%) to 87.814% when pulling 38c9311 on mschauer:SDiagonal into 4e1a70b on JuliaArrays:master.

andyferris · 2017-07-08T22:49:41Z

@andyferris I hope I addressed all your comments.

It's looking good. There are some more imports we could move to the main package file, that's about it that I see.

mschauer · 2017-07-09T07:32:42Z

I left them there because they are imported locally elsewhere: https://github.com/JuliaArrays/StaticArrays.jl/search?utf8=✓&q=import&type= We could centralize these imports independently.

andyferris · 2017-07-09T09:29:15Z

Fine - no worries then :)

andyferris · 2017-07-09T09:30:03Z

Thanks again

c42f · 2017-07-10T05:59:22Z

src/SDiagonal.jl

@@ -0,0 +1,106 @@
+# Originally contributed by D. Getz (https://github.com/getzdan), M. Schauer


By the way, if you're porting someone else's code, and you want to attribute them, it's also quite possible to make them the author in git using something like git commit --author="Some Body <[email protected]>"

When I do this kind of thing, I'd be inclined to add the original chunk of code (in probably non-working form) under the other author's name as a single commit (@mentioning them to make sure they're happy with that). Then add any necessary changes under your own name in further commits.

The nice thing about doing it this way is you avoid baking authorship into comments which people feel they can't remove (like the one above), but you also get to do proper attribution which I feel is quite important.

Just some thoughts, I'm happy this was merged already.

When you port entire files (like in this case) it is also possible to preserve the full git history http://gbayer.com/development/moving-files-from-one-git-repository-to-another-preserving-history/

fredrikekre · 2017-07-10T09:21:08Z

Ref JuliaLang/julia#22718 for a potential future rewrite to utilize Base.Diagonal for later Julia versions.

tkelman · 2017-07-11T04:10:40Z

test/SDiagonal.jl

+
+        # Bad input
+        @test_throws Exception SMatrix{1,Int}()
+        @test_throws Exception SMatrix{2,Int}((1,))


@test_throws Exception is generally a very bad idea. It will pass if the code has a typo.

* Porting SDiagonal from Bridge.jl Originally contributed by D. Getz (https://github.com/getzdan), M. Schauer at https://github.com/mschauer/Bridge.jl under MIT License * Inherit from AbstractMatrix * Fixes and tests for constructors * More tests, more fixes * Address andyferris' comments * SDiagonal: next round of comments * Remaining tests for SDiagonal * Where-notation in SDiagonal * Test for inv(::SDiagonal)

Porting SDiagonal from Bridge.jl

aebf3bc

Originally contributed by D. Getz (https://github.com/getzdan), M. Schauer at https://github.com/mschauer/Bridge.jl under MIT License

mschauer force-pushed the SDiagonal branch from 4f3081d to aebf3bc Compare July 2, 2017 08:26

Inherit from AbstractMatrix

1eeb98b

mschauer force-pushed the SDiagonal branch from 4be7637 to 1eeb98b Compare July 2, 2017 14:02

Fixes and tests for constructors

57f2526

mschauer force-pushed the SDiagonal branch from 724a8cf to 57f2526 Compare July 2, 2017 19:28

More tests, more fixes

0772fb6

andyferris requested changes Jul 3, 2017

View reviewed changes

Address andyferris' comments

1244a2c

mschauer force-pushed the SDiagonal branch from 8179c86 to 1244a2c Compare July 3, 2017 10:41

andyferris reviewed Jul 4, 2017

View reviewed changes

SDiagonal: next round of comments

ec68f06

mschauer force-pushed the SDiagonal branch from 8ff246c to ec68f06 Compare July 4, 2017 07:00

Remaining tests for SDiagonal

c41d829

mschauer force-pushed the SDiagonal branch from 4f943ef to c41d829 Compare July 4, 2017 22:26

mschauer added 2 commits July 5, 2017 00:36

Where-notation in SDiagonal

cf7b1da

Test for inv(::SDiagonal)

38c9311

andyferris merged commit 3e7c71e into JuliaArrays:master Jul 9, 2017

c42f reviewed Jul 10, 2017

View reviewed changes

tkelman reviewed Jul 11, 2017

View reviewed changes

mschauer deleted the SDiagonal branch July 18, 2017 17:44

tkelman referenced this pull request in mschauer/Bridge.jl Jul 18, 2017

use ConjugatePriors again and refactor one example

671414d

simonbyrne mentioned this pull request Oct 26, 2018

Replace SDiagonal with LinearAlgebra.Diagonal of SVector #531

Closed

oschulz pushed a commit to oschulz/StaticArrays.jl that referenced this pull request Apr 4, 2023

bump version (JuliaArrays#240)

ea83c8a



		convert{N,T}(::Type{SDiagonal{N,T}}, D::SDiagonal{N,T}) = D
		convert{N,T}(::Type{SDiagonal{N,T}}, D::SDiagonal) = SDiagonal{N,T}(convert(SVector{N,T}, D.diag))

		@@ -0,0 +1,106 @@
		# Originally contributed by D. Getz (https://github.com/getzdan), M. Schauer

Porting SDiagonal from Bridge.jl #240

Porting SDiagonal from Bridge.jl #240

Conversation

mschauer commented Jul 2, 2017 • edited Loading

mschauer commented Jul 2, 2017

coveralls commented Jul 2, 2017

codecov-io commented Jul 2, 2017 • edited Loading

Codecov Report

coveralls commented Jul 2, 2017

coveralls commented Jul 2, 2017

coveralls commented Jul 2, 2017

coveralls commented Jul 2, 2017

mschauer commented Jul 2, 2017

coveralls commented Jul 2, 2017

andyferris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mschauer commented Jul 3, 2017

coveralls commented Jul 3, 2017

coveralls commented Jul 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyferris commented Jul 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 4, 2017

coveralls commented Jul 4, 2017

coveralls commented Jul 4, 2017

coveralls commented Jul 4, 2017

mschauer commented Jul 4, 2017

coveralls commented Jul 4, 2017

coveralls commented Jul 4, 2017

andyferris commented Jul 8, 2017

mschauer commented Jul 9, 2017

andyferris commented Jul 9, 2017

andyferris commented Jul 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredrikekre commented Jul 10, 2017

Choose a reason for hiding this comment

mschauer commented Jul 2, 2017 •

edited

Loading

codecov-io commented Jul 2, 2017 •

edited

Loading