bring back fixedsizearrays #138

SimonDanisch · 2017-04-06T10:04:01Z

No description provided.

coveralls · 2017-04-06T10:14:58Z

Coverage decreased (-0.4%) to 69.191% when pulling d588e60 on sd/fixedsizearrays into 8d3ec32 on master.

SimonDanisch · 2017-04-06T10:18:37Z

Uh, this seems to still pass, but I pulled in newest master and it doesn't pass anymore...
Also master doesn't pass on newest Julia master. Is there some branch fixing this that I should include here?
Error:

Base.ArgumentError(msg="Size mismatch in Static Array parameters. Got size Tuple{1,2}, dimension 2 and length 1.")

@andyferris @c42f @Keno

SimonDanisch · 2017-04-06T11:26:08Z

Lol, just spent way too much time to figure out, that test_broken exists, doesn't leave a stacktrace, but turns up as a broken test in the summary -.-
I think I fixed the errors I mention above by putting the error message of the generated function in quotes:

StaticArrays.jl/src/util.jl

Line 42 in b1714d9

function check_array_parameters(Size, T, N, L)

, so that they don't make the body of the staged function impure!

coveralls · 2017-04-06T12:15:27Z

Coverage increased (+0.6%) to 70.213% when pulling 6b6f519 on sd/fixedsizearrays into 8d3ec32 on master.

coveralls · 2017-04-06T19:47:44Z

Coverage increased (+0.7%) to 70.274% when pulling 2433157 on sd/fixedsizearrays into 8d3ec32 on master.

c42f

This generally looks good, thanks!

Which parts of this do you still need? I'd like to think of the FixedSizeArrays submodule as something we can eventually remove, and have everyone using the same fixed size API.

c42f · 2017-04-06T21:08:04Z

src/FixedSizeArrays.jl

-        size_or{T}(::Type{$(name){TypeVar(:S), T}}, or) = or
-        size_or{S}(::Type{$(name){S, TypeVar(:T)}}, or) = (S,)
-        size_or{S, T}(::Type{$(name){S, T}}, or) = (S,)
+        if VERSION < v"0.6.0-dev"


StaticArrays as a whole is now 0.6 only. Is there any use in having this version check?

c42f · 2017-04-06T21:17:40Z

src/util.jl

    end

    if L != tuple_prod(Size) || L < 0 || tuple_minimum(Size) < 0 || tuple_length(Size) != N
-        throw(ArgumentError("Size mismatch in Static Array parameters. Got size $Size, dimension $N and length $L."))
+        return :(throw(ArgumentError("Size mismatch in Static Array parameters. Got size $Size, dimension $N and length $L.")))


I think I'm still a bit confused about why this is absolutely necessary. But I'm not surprised that this is another concession required for the new @generated rules :-)

I also don't get why this helps... I had actually removed the quotes from the errors in most parts of the package because they seemed to not be required...

Yes arguably it's better to throw an error early rather than late (ie, in the generator rather than the generated code - so to leave this without the quotes). I just figured it was some funky new limitation of @generated... @SimonDanisch what exactly happens without this?

c42f · 2017-04-06T21:30:09Z

src/util.jl

-    !isa(T, Type) && throw(ArgumentError("Static Array parameter T must be a type, got $T"))
-    !isa(N.parameters[1], Int) && throw(ArgumenError("Static Array parameter N must be an integer, got $(N.parameters[1])"))
-    !isa(L.parameters[1], Int) && throw(ArgumentError("Static Array parameter L must be an integer, got $(L.parameters[1])"))
+@generated function check_array_parameters(Size, T, N, L)


I don't think this needs to be @generated, because this method only exists as a fallback signature for when the valid check_array_parameters signature further down fails to match. So codegen does not need to be good for this one.

It took me five minutes to figure out why we even have two versions of check_array_parameters though. Would be good to add a comment along the lines of

# This version of check_array_parameters() is a fallback signature to catch cases when invalid type # parameters are passed. It exists only for improved error reporting and should not be called by # correct code.

Oh.... That probably wasn't intended... I got a bit confused because I started testing with an old version of master, where this was still a generated function...

c42f · 2017-04-06T21:33:15Z

src/StaticArrays.jl

@@ -93,7 +93,7 @@ include("eigen.jl")
 include("cholesky.jl")
 include("deque.jl")

-#include("FixedSizeArrays.jl")  # Currently defunct
+include("FixedSizeArrays.jl")  # Currently defunct


Obsolete comment.

SimonDanisch · 2017-04-10T09:55:50Z

Which parts of this do you still need?

All!

I wouldn't mind to have this come as standard functionality of StaticArrays!

coveralls · 2017-04-10T10:08:04Z

Coverage increased (+0.8%) to 70.427% when pulling 80342bf on sd/fixedsizearrays into 8d3ec32 on master.

c42f · 2017-04-11T01:22:19Z

I wouldn't mind to have this come as standard functionality of StaticArrays

Certainly it would be great to have any distinct functionality which still lives in here as part of StaticArrays proper, provided it doesn't conflict with Base.AbstractArray conventions. (For example, Base.isnan is an elementwise operation in julia 0.5, but FixedSizeArrays.isnan is a reduction. You could/should use any(isnan.(v)) instead.)

c42f

This all looks fine to me, with the exception of check_array_parameters, which I'm still a bit suspicious of ;-)

c42f · 2017-04-11T11:17:35Z

src/util.jl

+    (!isa(Size, DataType) || (Size.name !== Tuple.name)) && return throw(ArgumentError("Static Array parameter Size must be a Tuple type, got $Size"))
+    !isa(T, Type) && return throw(ArgumentError("Static Array parameter T must be a type, got $T"))
+    !isa(N.parameters[1], Int) && return throw(ArgumenError("Static Array parameter N must be an integer, got $(N.parameters[1])"))
+    !isa(L.parameters[1], Int) && return throw(ArgumentError("Static Array parameter L must be an integer, got $(L.parameters[1])"))


These returns don't do anything - leftovers from previous changes, I think?

Yeah, ditch the returns.

c42f · 2017-04-11T11:20:49Z

src/util.jl

 end

-@generated function check_array_parameters{Size,T,N,L}(::Type{Size}, ::Type{T}, ::Type{Val{N}}, ::Type{Val{L}})
+function check_array_parameters{Size,T,N,L}(::Type{Size}, ::Type{T}, ::Type{Val{N}}, ::Type{Val{L}})


This is the version of check_array_parameters() which will be called, and I think there was a reason to have it as a @generated function - it's fairly important that this does compile down to nothing in the case that the checks pass.

Did you check that this actually compiles down to nothing, even when it's not a generated function? I'd suggest you just leave this as it was, but include the fix you had before, to throw in the generated code, rather than in the generator itself.

Oh I see where my confusion comes from... Only one of them where @generated .
It's a pity, that Julia's dead code elimination just removes this in theory...

Right, this is why I suggested adding that explanatory comment in my previous review of this PR ;-)

Yeah this is a bit annoying. Generally the compiler tries to compute things which are either pure or "effect free" and perform dead branch elimination, but I think this function is just too complex. However inference is getting better all the time at this so its worth keeping an eye on in the future.

SimonDanisch · 2017-04-11T11:37:38Z

FixedSizeArrays.isnan is a reduction. You could/should use any(isnan.(v)) instead.)

Not 100% sure about that, now that people should use isnan.(Vector) for element wise application, which should disambiguate this case... Also, I feel like there are tons of examples, where StaticVectors are rightfully treated as scalars.
But I guess I can remove it, if that would make it hard to merge for you!

c42f · 2017-04-11T12:45:20Z

Not 100% sure about that, now that people should use isnan.(Vector) for element wise application, which should disambiguate this case

True, once 0.6 deprecations are of the way isnan(::Vector) could take on a new meaning in Base, in principle. Though I'm not sure it should - as a Base function, would it mean all(isnan.(v)) or any(isnan.(v)) for instance? Seems potentially ambiguous.

But I guess I can remove it, if that would make it hard to merge for you!

Not at all, I'm fine merging it inside the StaticArrays.FixedSizeArrays submodule. I just meant this as an example of where the utilities in StaticArrays.FixedSizeArrays diverge from AbstractArray, which would make me uncomfortable about including them in the main StaticArrays module.

SimonDanisch · 2017-04-18T13:05:18Z

Should we merge it as is and get rid of the fixedsizearray module in a later release?

andyferris · 2017-04-19T03:48:08Z

FYI the reductions generally take a function, like any(isnan, vector) (much faster than any(isnan.(vector)))

andyferris · 2017-04-19T03:59:11Z

Simon - I'm happy that you have worked on this. Let's fix (revert?) util.jl and merge.

andyferris · 2017-04-19T04:00:37Z

Actually, can you explain the broadcast changes?

(Would it have been better to have generic bugfixes in a separate PR?)

Tests have not been updated

Still having an inference problem in test/solve.jl. Can't reproduce, however.

SimonDanisch · 2017-04-19T09:36:02Z

Actually, can you explain the broadcast changes?
(Would it have been better to have generic bugfixes in a separate PR?)

Yes I guess... Problem is, it's much nicer (for me) to have all fixes for GeometryTypes/GLVisualize in one branch....

The change makes round.(Int, vec) work and opens up support for similar behaviour to what base is doing - treating everything not an array as a scalar!

andyferris · 2017-04-19T09:49:37Z

I thought so - got it!

Base in v0.6 actually has a rather fancy implementation of broadcast. Ideally we'd hook into this (assuming it is inlined).

coveralls · 2017-04-19T10:11:14Z

Coverage increased (+0.9%) to 70.472% when pulling 8c8a5ca on sd/fixedsizearrays into 8d3ec32 on master.

SimonDanisch · 2017-04-19T10:11:47Z

ooooh, that'd be sweet :)

I checked out utils.jl and only put the error in quotes for the generated function... otherwise the error looks just plain horrible!

c42f · 2017-04-19T11:16:46Z

I checked out utils.jl and only put the error in quotes for the generated function

Thanks Simon, this looks a lot better now.

c42f

This looks good to me now 👍

andyferris · 2017-04-19T11:37:57Z

Simon - I'd really like to also merge #134. However, it is a bit breaking... Because I am lazy 😄, could I ask you to (a) give your opinion of #134, and (b) if you like it, consider rebasing this upon that?

SimonDanisch · 2017-04-19T13:04:19Z

Okay, that was way too easy :D

coveralls · 2017-04-19T13:12:02Z

Coverage increased (+0.4%) to 70.115% when pulling 380c44b on sd/fixedsizearrays into b643ff5 on master.

andyferris · 2017-04-21T04:25:34Z

This needs a rebase

SimonDanisch · 2017-04-24T12:16:23Z

Done!

coveralls · 2017-04-24T12:34:30Z

Coverage increased (+0.7%) to 70.636% when pulling e1f68b1 on sd/fixedsizearrays into c4008ff on master.

SimonDanisch added 2 commits April 6, 2017 12:02

bring back fixedsizearrays

d588e60

fix missing inds_expr

bee237d

Merge branch 'master' into sd/fixedsizearrays

924c632

SimonDanisch added 3 commits April 6, 2017 13:26

defer error into function body

8df9f26

fix and remove depwarns

3663d29

don't use compat

6b6f519

allow more types for _broadcast + fix round

2433157

c42f reviewed Apr 6, 2017

View reviewed changes

clean up

80342bf

c42f reviewed Apr 11, 2017

View reviewed changes

SimonDanisch mentioned this pull request Apr 18, 2017

switch to StaticArrays and get 0.6 working [WIP] JuliaGeometry/GeometryTypes.jl#80

Merged

Andy Ferris added 3 commits April 19, 2017 14:06

Add size to abstract StaticArray type

a000ab2

Tests have not been updated

Fixed some subtyping issues

a0418f6

More bugfixes.

2086cb1

Still having an inference problem in test/solve.jl. Can't reproduce, however.

remove return

8c8a5ca

c42f approved these changes Apr 19, 2017

View reviewed changes

c42f mentioned this pull request Apr 19, 2017

Tag a new release? #142

Closed

SimonDanisch added 2 commits April 19, 2017 14:43

Merge branch 'size-in-staticarray-type' into sd/fixedsizearrays

ed10d57

merge #134

380c44b

SimonDanisch added 2 commits April 24, 2017 14:02

test isnan

ff6ebc4

merge master

e1f68b1

c42f merged commit 2a09e2c into master Apr 24, 2017

SimonDanisch deleted the sd/fixedsizearrays branch April 25, 2017 08:03

bring back fixedsizearrays #138

bring back fixedsizearrays #138

Uh oh!

Conversation

SimonDanisch commented Apr 6, 2017

Uh oh!

coveralls commented Apr 6, 2017

Uh oh!

SimonDanisch commented Apr 6, 2017

Uh oh!

SimonDanisch commented Apr 6, 2017

Uh oh!

coveralls commented Apr 6, 2017

Uh oh!

coveralls commented Apr 6, 2017

Uh oh!

c42f left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SimonDanisch commented Apr 10, 2017

Uh oh!

coveralls commented Apr 10, 2017

Uh oh!

c42f commented Apr 11, 2017

Uh oh!

c42f left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SimonDanisch commented Apr 11, 2017

Uh oh!

c42f commented Apr 11, 2017

Uh oh!

SimonDanisch commented Apr 18, 2017

Uh oh!

andyferris commented Apr 19, 2017

Uh oh!

andyferris commented Apr 19, 2017

Uh oh!

andyferris commented Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SimonDanisch commented Apr 19, 2017

Uh oh!

andyferris commented Apr 19, 2017

Uh oh!

coveralls commented Apr 19, 2017

Uh oh!

SimonDanisch commented Apr 19, 2017

Uh oh!

c42f commented Apr 19, 2017

Uh oh!

c42f left a comment

Choose a reason for hiding this comment

Uh oh!

andyferris commented Apr 19, 2017 •

edited

Loading