Julia 0.6 support / updates to match StaticArrays 0.4.0 #25

tkoolen · 2017-03-25T16:55:23Z

The easy part of adding support for Julia 0.6. See commit messages.

The problem I ran into is an ambiguity related to https://github.com/JuliaArrays/StaticArrays.jl/blob/b935326703d2aab13a33efd4b71bac07f86ea376/src/convert.jl#L4:

julia> using StaticArrays, Rotations

julia> m = eye(SMatrix{2, 2, Float32})
2×2 StaticArrays.SMatrix{2,2,Float32,4}:
 1.0  0.0
 0.0  1.0

julia> RotMatrix(m)
ERROR: MethodError: Rotations.RotMatrix{2,Float32,4}(::StaticArrays.SMatrix{2,2,Float32,4}) is ambiguous. Candidates:
  (::Type{Rotations.RotMatrix{N,T,L}})(mat) where {N, T, L} in Rotations at /Users/twan/code/julia/RigidBodyDynamics/v0.6/Rotations/src/core_types.jl:77
  (::Type{SA})(a::AbstractArray) where SA<:StaticArrays.StaticArray in StaticArrays at /Users/twan/code/julia/RigidBodyDynamics/v0.6/StaticArrays/src/convert.jl:4
Stacktrace:
 [1] Rotations.RotMatrix(::StaticArrays.SMatrix{2,2,Float32,4}) at /Users/twan/code/julia/RigidBodyDynamics/v0.6/Rotations/src/core_types.jl:77

I wasn't sure how to handle this. I guess this is an issue for any concrete StaticArray subtype whose only field is another StaticArray.

andyferris · 2017-03-25T21:11:09Z

src/core_types.jl


 A statically-sized, N×N unitary (orthogonal) matrix.

 Note: the orthonormality of the input matrix is *not* checked by the constructor.
 """
-immutable RotMatrix{N,T,L} <: Rotation{N,T} # which is <: AbstractMatrix{T}
+struct RotMatrix{N,T,L} <: Rotation{N,T} # which is <: AbstractMatrix{T}
    mat::SMatrix{N, N, T, L} # The final parameter to SMatrix is the "length" of the matrix, 3 × 3 = 9


So should we override the default constructors to fix the ambiguity?

Something like

struct RotMatrix{N,T,L} <: Rotation{N,T} # which is <: AbstractMatrix{T} mat::SMatrix{N, N, T, L} # The final parameter to SMatrix is the "length" of the matrix, 3 × 3 = 9 RotMatrix{N,T,L}(x::AbstractArray) where {N,T,L} = new{N,T,L}(convert(SMatrix{N,N,T,L}, x)) end

That's a stack overflow error:

[1] macro expansion at /Users/twan/code/julia/RigidBodyDynamics/v0.6/StaticArrays/src/SMatrix.jl:65 [inlined] [2] Type at /Users/twan/code/julia/RigidBodyDynamics/v0.6/StaticArrays/src/SMatrix.jl:61 [inlined] [3] Type at /Users/twan/code/julia/RigidBodyDynamics/v0.6/Rotations/src/core_types.jl:86 [inlined] [4] convert at /Users/twan/code/julia/RigidBodyDynamics/v0.6/StaticArrays/src/convert.jl:7 [inlined] [5] Rotations.RotMatrix(::StaticArrays.SMatrix{2,2,Float32,4}) at /Users/twan/code/julia/RigidBodyDynamics/v0.6/StaticArrays/src/convert.jl:4 (repeats 80000 times)

andyferris · 2017-03-25T21:21:12Z

src/core_types.jl


 A statically-sized, N×N unitary (orthogonal) matrix.

 Note: the orthonormality of the input matrix is *not* checked by the constructor.
 """
-immutable RotMatrix{N,T,L} <: Rotation{N,T} # which is <: AbstractMatrix{T}
+struct RotMatrix{N,T,L} <: Rotation{N,T} # which is <: AbstractMatrix{T}
    mat::SMatrix{N, N, T, L} # The final parameter to SMatrix is the "length" of the matrix, 3 × 3 = 9
 end



Then we probably need to redefine the default outer constructor (which is lost because we defined an inner constructor).

RotMatrix(x::SMatrix{N.N,T,L}) where {N,T,L} = RotMatrix{N,T,L}(x)

Ideally, we could have a series of constructors to populate N, T, etc, so that RotMatrix{2}([1 0; 0 1]) works (maybe useful for REPL...)

Adding

RotMatrix(x::SMatrix{N,N,T,L}) where {N,T,L} = RotMatrix{N,T,L}(x)

in addition to the inner constructor does fix the ambiguity and stack overflow error; sorry, I should have tried that before commenting earlier.

andyferris · 2017-03-25T21:26:08Z

Thanks @tkoolen - what I've seen so far looks good. This is really appreciated :)

…guity errors.

tkoolen · 2017-03-27T19:15:48Z

The next issue is the convert methods for the Euler types. I've made some changes to RotX. Before making the same changes to all of the other Euler types, could you take a look?

I made sure that the following works:

r = RotX(Float32(3.))
RotX(r)
RotX{Float32}(r)
RotX{Float64}(r)
convert(RotX{Float32}, r)
convert(RotX{Float64}, r)
convert(RotX, r)

Given the need for these changes, I'm starting to think that the answer to this question might be 'no'... What do you think?

tkoolen · 2017-03-30T19:42:08Z

I went ahead and made the same changes I made to RotX throughout. In doing so, I reduced the code duplication in euler_types.jl using some simple metaprogramming. I also worked around JuliaArrays/StaticArrays.jl#128.

andyferris · 2017-03-31T01:06:15Z

You shouldn't need to work around JuliaArrays/StaticArrays.jl#128 - see JuliaArrays/StaticArrays.jl#130

tkoolen · 2017-03-31T01:12:01Z

OK. Let's undo the workaround once a new StaticArrays tag is in, so that this can target 0.4.0.

tkoolen · 2017-03-31T01:12:10Z

src/derivatives.jl

@@ -36,7 +36,8 @@ function jacobian(::Type{RotMatrix},  q::Quat)
    # then R = RotMatrix(q) = RotMatrix(s * qhat) = s * RotMatrix(qhat)

    # get R(q)
-    R = q[:]


For future reference, this is the only place where JuliaArrays/StaticArrays.jl#128 was causing issues.

Fixed upstream, will need a StaticArrays-0.4.1 release though

c42f

Great refactor, thanks @tkoolen!

c42f · 2017-03-31T03:24:21Z

src/euler_types.jl

-    return R(2*pi*rand(T))
+    # Not really sure what this distribution is, but it's also not clear what
+    # it should be! rand(RotXY) *is* invariant to pre-rotations by a RotX and
+    # post-rotations by a RotY...


Good point. If it's not a known distribution, I'd guess

Experts won't use it

Naive users will use it and get something unexpected :-)

Perhaps better just to remove this entirely - people can easily write rand(RotX)*rand(RotY) and get the same thing with improved clarity about which exact distribution they're using.

This was not my comment; I just copied it from the original location.

Right, I guess this PR is probably not the best place to be removing things anyway.

c42f · 2017-03-31T03:27:51Z

src/euler_types.jl

+    end
+end
+
+function Base.rand{R <: Union{RotX,RotY,RotZ}}(::Type{R})


I wonder whether it would it be productive to define SingleAxisRotation = Union{RotX,RotY,RotZ}, and move most of the definitions in the loop down here instead, just like rand?

It would probably lead to more straightforward code, and a lot less entries added to method tables (though I'm not sure whether this is ultimately good or bad for the julia runtime when the methods which are added have complex union types as parameters :-) ).

Either way for me. I don't currently have a good mental model for costs related to method tables. @andyferris, any thoughts?

Well, let's not let this block merging the fixes.

c42f · 2017-04-03T01:22:47Z

@tkoolen what's the status of this PR now that you've added various changes? Did you sort out the queries further up the page, or do you still want another opinion?

tkoolen · 2017-04-03T01:27:36Z

Ready to go in my opinion. There's no new StaticArrays tag yet, so the workaround is still needed for now.

c42f · 2017-04-03T01:38:43Z

Excellent.

I'm not super keen on dropping julia-0.5 support in Rotations before julia-0.6 is out... but given that StaticArrays is in a state of flux I guess we'd better just merge this and move forward :-)

tkoolen · 2017-04-03T02:01:00Z

Great, thanks!

timholy · 2017-04-11T13:33:41Z

Tag a new release? JuliaImages/ImageTransformations.jl#18

timholy · 2017-04-11T13:34:04Z

Oh, and many thanks, @tkoolen!

tkoolen · 2017-04-11T15:20:45Z

@timholy, see JuliaLang/METADATA.jl#8787.

tkoolen added 5 commits March 25, 2017 12:14

New abstract type syntax.

385fbf1

New inner constructor syntax.

2630fc4

Dot syntax for abs applied to arrays.

f87aa09

::Int instead of ::Integer in getindex methods, following StaticArrays.

52d109e

Change immutable to struct.

43343c9

andyferris reviewed Mar 25, 2017

View reviewed changes

tkoolen added 4 commits March 27, 2017 13:47

Fix takebuf_string deprecation in test.

bd15ab9

Tentatively add inner and outer constructors to RotMatrix to fix ambi…

f24f6f1

…guity errors.

Use convert(Tuple, ...) to fix infinite recursion.

e2aa085

Tentative fix for RotX ambiguity errors.

8c067e7

tkoolen added 3 commits March 30, 2017 15:18

Apply changes to RotX throughout, reduce duplication.

955fbbf

Work around StaticArrays getindex issue.

1eb9545

Update .travis.yml, REQUIRE.

aae8f8f

tkoolen changed the title ~~Mindless part of adding Julia 0.6 support / updates to match StaticArrays master~~ Julia 0.6 support / updates to match StaticArrays 0.4.0 Mar 30, 2017

Undo docstring indentation changes.

0dbaee9

tkoolen commented Mar 31, 2017

View reviewed changes

c42f approved these changes Mar 31, 2017

View reviewed changes

c42f merged commit 1442562 into JuliaGeometry:master Apr 3, 2017

tkoolen deleted the tk-julia-0.6 branch April 3, 2017 02:00

tkoolen mentioned this pull request Apr 3, 2017

On Julia v0.6 the size of rotations are not known (due to StaticArrays) #24

Closed

tkoolen mentioned this pull request Apr 11, 2017

Julia 0.6 support issue tracker JuliaRobotics/RigidBodyDynamics.jl#203

Closed

7 tasks

tkoolen mentioned this pull request Jul 27, 2017

Add missing Tuple constructor #38

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Julia 0.6 support / updates to match StaticArrays 0.4.0 #25

Julia 0.6 support / updates to match StaticArrays 0.4.0 #25

tkoolen commented Mar 25, 2017

andyferris Mar 25, 2017 •

edited

Loading

tkoolen Mar 26, 2017

andyferris Mar 25, 2017 •

edited

Loading

andyferris Mar 25, 2017

tkoolen Mar 27, 2017

andyferris commented Mar 25, 2017

tkoolen commented Mar 27, 2017

tkoolen commented Mar 30, 2017

andyferris commented Mar 31, 2017

tkoolen commented Mar 31, 2017

tkoolen Mar 31, 2017

c42f Mar 31, 2017

c42f left a comment

c42f Mar 31, 2017

tkoolen Mar 31, 2017

c42f Mar 31, 2017

c42f Mar 31, 2017

tkoolen Mar 31, 2017

c42f Apr 3, 2017

c42f commented Apr 3, 2017

tkoolen commented Apr 3, 2017

c42f commented Apr 3, 2017

tkoolen commented Apr 3, 2017

timholy commented Apr 11, 2017

timholy commented Apr 11, 2017

tkoolen commented Apr 11, 2017

Julia 0.6 support / updates to match StaticArrays 0.4.0 #25

Julia 0.6 support / updates to match StaticArrays 0.4.0 #25

Conversation

tkoolen commented Mar 25, 2017

andyferris Mar 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyferris Mar 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyferris commented Mar 25, 2017

tkoolen commented Mar 27, 2017

tkoolen commented Mar 30, 2017

andyferris commented Mar 31, 2017

tkoolen commented Mar 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

c42f commented Apr 3, 2017

tkoolen commented Apr 3, 2017

c42f commented Apr 3, 2017

tkoolen commented Apr 3, 2017

timholy commented Apr 11, 2017

timholy commented Apr 11, 2017

tkoolen commented Apr 11, 2017

andyferris Mar 25, 2017 •

edited

Loading

andyferris Mar 25, 2017 •

edited

Loading