Rewrite all `@pure` functions #105

andyferris · 2017-02-15T12:23:57Z

Now we use Size more consistently and rely less on @pure implementations of similar_type.
similar and similar_type both use Size not integers to discuss size
similar and similar_type are now streamlined. Users only need to
override one definition, just like in Base.
Associated documentation and test changes.

* Now we use `Size` more consistently and rely less on `@pure` implementations of `similar_type`. * `similar` and `similar_type` both use `Size` not integers to discuss size * `similar` and `similar_type` are now streamlined. Users only need to override one definition, just like in Base. * Associated documentation and test changes.

andyferris · 2017-02-15T12:28:29Z

This was a little tricky to get inference working well on v0.5. I had to use @generated in two places where on v0.6- @pure was sufficient - so it's good that the compiler is improving :)

But in general I think this reorganization is a very good thing.

coveralls · 2017-02-15T12:50:35Z

Coverage increased (+1.06%) to 68.563% when pulling 5177a27 on ajf/nopure into 1be47cc on master.

andyferris · 2017-02-16T03:34:59Z

Test failure is caused by JuliaLang/julia#20620 on Julia master (v0.5 works).

(Ideally we would deal with this more fully but it seems orthogonal to this PR).

coveralls · 2017-02-16T12:48:02Z

Coverage increased (+3.9%) to 71.389% when pulling e600e89 on ajf/nopure into 1be47cc on master.

c42f

Great effort Andy, this looks like a fair bit of work.

The result is a nice improvement, I think. I like how Size is fitting in here.

c42f · 2017-02-20T01:03:56Z

src/MArray.jl

+@pure Size{S}(::Type{MArray{S}}) = Size(S)
+@pure Size{S,T}(::Type{MArray{S,T}}) = Size(S)
+@pure Size{S,T,N}(::Type{MArray{S,T,N}}) = Size(S)
+@pure Size{S,T,N,L}(::Type{MArray{S,T,N,L}}) = Size(S)


Do you know whether this kind of repetition go away with the new type system work in 0.6?

I think Size(::Type{T} where T <: MArray{S}) where S = Size(S) might be sufficient for all of the above, but I'd like to check. We could probably successfully move the @pure to the Size constructor and drop it from here, also.

c42f · 2017-02-20T01:26:03Z

src/abstractarray.jl

-@inline similar{SV <: StaticVector}(::SV) = MVector{length(SV),eltype(SV)}()
-@inline similar{SV <: StaticVector, T}(::SV, ::Type{T}) = MVector{length(SV),T}()
+similar{SA<:StaticArray,T}(::SA,::Type{T}) = similar(SA,T,Size(SA))
+similar{SA<:StaticArray,T}(::Type{SA},::Type{T}) = similar(SA,T,Size(SA))


So now we've got similar based on input StaticArray type as well as value. I guess that makes sense as they all still return a value which can be written into. Did did we have it before anywhere?

I think so (or we should have) but it was all a bit dodgier...

c42f · 2017-02-20T01:28:19Z

src/core.jl

-   a `MVector`, `MMatrix` or `MArray`. You will benefit from overloading this if
-   your container is already mutable!
- - `setindex!` on mutable arrays (e.g. those returned by `similar`)
+ - `setindex!` for a single elmenent (linear indexing).


c42f · 2017-02-20T01:30:07Z

src/deque.jl

 @generated function push(vec::StaticVector, x)
-    newtype = similar_type(vec, (length(vec) + 1 ,))
+    newtype = similar_type(vec, Size(length(vec) + 1 ,))


Minor tidy up: trailing comma no longer required here and in the rest of deque.jl.

c42f · 2017-02-20T01:31:57Z

src/linalg.jl

@@ -54,7 +54,7 @@ end
        error("Dimension mismatch")
    end

-    exprs = [i == j ? :(a.λ + b[$(sub2ind(size(a), i, j))]) : :(b[$(sub2ind(size(a), i, j))]) for i = 1:n, j = 1:n]
+    exprs = [i == j ? :(a.λ + b[$(sub2ind(size(b), i, j))]) : :(b[$(sub2ind(size(b), i, j))]) for i = 1:n, j = 1:n]


I guess this never worked before?

No... we need to work to improve test coverage.

c42f · 2017-02-20T01:37:11Z

test/FieldVector.jl

@@ -18,19 +18,21 @@
        @test length(p) === 3
        @test eltype(p) === Float64

+        @test (p + p)::Point3D ≈ Point3D(2.0, 4.0, 6.0)


Can use === here, I think, to check that both the elements and type are the same. (Elements should sum exactly, as the result is exactly representable in Float64.)

I never know when to trust floating point arithmetic

The rules say that basic floating point arithmetic operations should be correctly rounded. In particular, this means that if all intermediate results of a computation are exactly representable, then the final result is exact.

This certainly holds true when you're adding, subtracting and multiplying exact integers using floating point arithmetic, up until maxintfloat(Float64) == 9.007199254740992e15.

The reason I bring this up is that isapprox uses quite a generous default for rtol, a lot looser than you'd really want in many tests.

c42f · 2017-02-20T01:45:12Z

test/linalg.jl

-        @test @SMatrix([1 2 3; 4 5 6])' === @SMatrix([1 4; 2 5; 3 6])
+        @test @inferred(ctranspose(@SVector([1, 2, 3]))) === @SMatrix([1 2 3])
+        @test @inferred(ctranspose(@SMatrix([1 2; 0 3]))) === @SMatrix([1 0; 2 3])
+        @test @inferred(ctranspose(@SMatrix([1 2 3; 4 5 6]))) === @SMatrix([1 4; 2 5; 3 6])


That's a lot of @inferred. You need a custom @testinferred macro or some kind of helper to make these easier to look at ;-)

c42f · 2017-02-20T01:46:07Z

test/linalg.jl

@@ -82,4 +114,8 @@

        @test normalize(SVector(1,2,3)) ≈ normalize([1,2,3])
    end
+
+    @testset "trace" begin
+        @test trace(@SMatrix [1.0 2.0; 3.0 4.0]) ≈ 5.0


Can be exact equality, not \approx

c42f · 2017-02-20T01:50:17Z

I assume the CI failures in latest 0.6 are expected for now?

coveralls · 2017-02-20T23:41:05Z

Coverage increased (+3.9%) to 71.389% when pulling 6344b4f on ajf/nopure into 1be47cc on master.

andyferris force-pushed the ajf/nopure branch from cff869b to 5177a27 Compare February 15, 2017 12:25

andyferris mentioned this pull request Feb 15, 2017

incorrect pure annotations #95

Closed

andyferris requested a review from c42f February 16, 2017 03:12

andyferris mentioned this pull request Feb 16, 2017

Dispatch issues with size() on v0.6-dev #106

Closed

andyferris mentioned this pull request Feb 19, 2017

Use new inner constructor and type declaration syntax #107

Merged

c42f reviewed Feb 20, 2017

View reviewed changes

Added some more tests, addressed code review

6344b4f

andyferris force-pushed the ajf/nopure branch from e600e89 to 6344b4f Compare February 20, 2017 22:57

andyferris merged commit e6d7387 into master Feb 20, 2017

tkoolen mentioned this pull request Feb 23, 2017

Adapt to Size trait changes introduced in StaticArrays 0.3. JuliaGeometry/Rotations.jl#23

Merged

SimonDanisch deleted the ajf/nopure branch May 17, 2018 10:57

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

document widen (JuliaArrays#105)

ef21985

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite all `@pure` functions #105

Rewrite all `@pure` functions #105

andyferris commented Feb 15, 2017

andyferris commented Feb 15, 2017

coveralls commented Feb 15, 2017

andyferris commented Feb 16, 2017 •

edited

Loading

coveralls commented Feb 16, 2017

c42f left a comment

c42f Feb 20, 2017

andyferris Feb 20, 2017

c42f Feb 20, 2017

andyferris Feb 20, 2017

c42f Feb 20, 2017

c42f Feb 20, 2017

c42f Feb 20, 2017

andyferris Feb 20, 2017

c42f Feb 20, 2017

andyferris Feb 20, 2017

c42f Feb 20, 2017

c42f Feb 20, 2017

andyferris Feb 20, 2017

c42f Feb 20, 2017

c42f commented Feb 20, 2017

coveralls commented Feb 20, 2017

Rewrite all @pure functions #105

Rewrite all @pure functions #105

Conversation

andyferris commented Feb 15, 2017

andyferris commented Feb 15, 2017

coveralls commented Feb 15, 2017

andyferris commented Feb 16, 2017 • edited Loading

coveralls commented Feb 16, 2017

c42f 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

c42f commented Feb 20, 2017

coveralls commented Feb 20, 2017

Rewrite all `@pure` functions #105

Rewrite all `@pure` functions #105

andyferris commented Feb 16, 2017 •

edited

Loading