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

WIP/RFC: Implement SA[...] syntax for SArray literals #633

Merged
merged 2 commits into from
Sep 15, 2019
Merged

Conversation

c42f
Copy link
Member

@c42f c42f commented Jul 18, 2019

Here's a bare bones implementation of a very old idea, originally brought up by @andyferris in #24 (comment). Also related to the #518 discussion at #518 (comment).

The idea is to give a more attractive syntax for short SArray literals to make removing the multi-argument constructors (as suggested by @timholy in #518 (comment)) less of a bitter pill to swallow.

Some examples:

SA[1,2]  # Construct SVector{2,Int}
SA[1 2;  # Construct SMatrix{2,2,Int}
   3 4]

# For constructing other StaticArray subtypes, we can compose via conversion, rather than
# using a zoo of macros `@MMatrix`, `@MVector` etc etc.
MMatrix(SA[1 2;
           3 4])

# For example, from issue #626, a 3x3 stress tensor
Stress(SA[1 2 3;
          4 5 6;
          7 8 9])

I've spent some time worrying about the fact that this is technically this is a syntax pun: the result of the SA[...] depends on whether the name SA is bound to StaticArrays.SA or is local variable of some other type. However I've come to the conclusion that this is no worse than Int[1,2,3], which is also a pun on the array indexing syntax. Cases where this syntax could refer to something other than StaticArrays.SA are detectable by the compiler and should be accessible to a linter as well.

Possible extensions / todo:

@coveralls
Copy link

coveralls commented Jul 18, 2019

Coverage Status

Coverage increased (+0.03%) to 81.576% when pulling 2fead27 on cjf/rework-init into f669465 on master.

@c42f c42f force-pushed the cjf/rework-init branch from 6854803 to 479c86d Compare July 18, 2019 10:59
@c42f c42f mentioned this pull request Jul 19, 2019
@c42f
Copy link
Member Author

c42f commented Jul 22, 2019

So well... for something I expected to be a contentious issue there's been a deafening silence here so far :-)

Any thoughts @andyferris on the specifics of this? It's not very featureful compared to @SArray; in particular doesn't support syntax for comprehensions and calls to rand. But the comprehension support in SArray looks dodgy to me anyway (it uses eval?!). I think you also said yesterday that you'd rather have things like rand(Size(3,3)) as a generic replacement for @SArray rand(3,3)?

@andyferris
Copy link
Member

Thanks Chris.

I'm definitely very interested in this. I've been weighing up the difference between SA[...] and @SA[...].

The main distinction that I can think of is that we can support generators/comprehensions through the syntax @SA[f(x) for x in a]. On the other hand, the way that SA[f(x) for x in a] lowers is quite complex and completely tied to Array - in fact I can't see any way for SA[f(x) for x in a] to not give a mysterious error, unless we do something completely disgusting like overiding constructors for Array{SA, N} to throw an error and so no-one can construct it.

In the macro implementation, I suspect the existing eval call could be replaced with more modern constant propagation. Yes, I would drop support for zeros and rand and related; these seem unnecessary these days.

@andyferris
Copy link
Member

I played with an eval-free comprehension implementation of @SA in #636.

@andyferris
Copy link
Member

and calls to rand

For the record, I think the macros supporting calls to rand and zeros and so-on is a mistake and we don't need that functionality (using e.g. zeros(Size(3)) or zeros(SOneTo(3)) seems sufficient to me).

@c42f
Copy link
Member Author

c42f commented Jul 22, 2019

On the other hand, the way that SA[f(x) for x in a] lowers is quite complex and completely tied to Array

Yeah, the lowering of typed comprehension seems kind of awful. On the other hand, it might be possible or even easy to change that by adjusting lowering. There's a "TODO this is a hack..." comment in the associated lowering code.

@tkf
Copy link
Member

tkf commented Jul 22, 2019

How about SA(f(x) for x in a)? I guess it wouldn't be hard to define SA(::Base.Generator).

@andyferris
Copy link
Member

Well, we can support SArray(f(x) for x in SOneTo(3)) for example. But due to the way constant propagation works we won't be getting SArray(f(x) for x in 1:3) working fast anytime soon, when f is a non-const closure (which are some of the occassions where a comprehension/generator is the most useful). Not sure if that is a deal-breaker for anyone?

@tkf
Copy link
Member

tkf commented Jul 23, 2019

It looks like you can make it work if it's just UnitRange?

julia> f() = _f(x^2 for x in 1:3)
f (generic function with 1 method)

julia> _f(xs) = ntuple(x -> xs.f(x + xs.iter.start - 1), xs.iter.stop)
_f (generic function with 1 method)

julia> @code_warntype optimize=true f()
Variables
  #self#::Core.Compiler.Const(f, false)
  #3::getfield(Main, Symbol("##3#4"))

Body::Tuple{Int64,Int64,Int64}
1return (1, 4, 9)

Maybe you mean it doesn't work with generic literals (e.g., SArray(f(x) for x in "abc"))? But supporting only UnitRange and StepRange could be good enough?

@andyferris
Copy link
Member

@tkf the case I am thinking of is where the closure (in your case x -> x^2) contains run-time data meaning the Generator becomes non-const (Julia doesn't track partially-constant structs where some fields are known and others are not). For example replace f with:

f(y) = _f(x^2 + y for x in 1:3)

and try look at e.g. @code_typed f(3)

@tkf
Copy link
Member

tkf commented Jul 24, 2019

Ah, I see. I wasn't paying attention to the non-const'ness of the struct field in your first reply. But luckily for us, it looks like it actually works in 1.2 (and master)?

               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.2.0-rc2.0 (2019-07-08)
 _/ |\__'_|_|_|\__'_|  |
|__/                   |

julia> f(y) = _f(x^2 + y for x in 1:3)
f (generic function with 1 method)

julia> _f(xs) = ntuple(x -> xs.f(x + xs.iter.start - 1), xs.iter.stop)
_f (generic function with 1 method)

julia> @code_typed optimize=true f(3)
CodeInfo(
1 ─ %1 = Base.add_int(1, y)::Int64
│   %2 = Base.add_int(4, y)::Int64
│   %3 = Base.add_int(9, y)::Int64
│   %4 = Core.tuple(%1, %2, %3)::Tuple{Int64,Int64,Int64}
└──      return %4
) => Tuple{Int64,Int64,Int64}

Though it doesn't work in 1.1 as you said.

@andyferris
Copy link
Member

OK, well that is pretty exciting, if there are partially-const struct lattice elements in inference now, then that opens up tons of possibilities all over the place.

@andyferris
Copy link
Member

andyferris commented Jul 24, 2019

I have direct confirmation from @Keno - it seems partially-const propagation now works anywhere that normal const propagation would (including for non-isbits structs). 🎉

@c42f
Copy link
Member Author

c42f commented Jul 24, 2019

Yeah that's pretty crazy. So it looks like we could have this version.

Possibly with a tweak to the way typed comprehensions are lowered by Base we could also have the typed comprehension syntax. That extension would be in julia-1.3 at earliest but I think we can easily wait for that as long as there's some kind of clear path forward to consistency.

@c42f
Copy link
Member Author

c42f commented Jul 25, 2019

I've implemented the more general SA{T} syntax and done some cleanup.

I'm wondering whether we actually need to support generator syntax with SA or whether that would be better handled by adding plain old constructors to SArray and MArray, etc. For instance, having

SArray(i for i=1:3)

seems fairly reasonable and doesn't fall afoul of #518. SA is largely about having an attractive answer to the multi-argument constructor problems.

@c42f
Copy link
Member Author

c42f commented Jul 27, 2019

Modifying lowering to make typed comprehension turn into something we can hook seems like it was just a matter of deleting a (hopefully obsolete) workaround: JuliaLang/julia#32709.

@c42f c42f added design speculative design related issue feature features and feature requests labels Aug 1, 2019
@c42f c42f added this to the 1.0 milestone Aug 1, 2019
@andyferris
Copy link
Member

cc @EvertSchippers @MeganDawson

@c42f
Copy link
Member Author

c42f commented Sep 11, 2019

@andyferris this has been slowly bit-rotting but coming back to look at it again I think we should just merge it and see how it feels.

As noted above, I think any worries about supporting generator syntax are orthogonal and should just be left to constructors + constant propagation.

(Somewhat OT, but FWIW I still think JuliaLang/julia#32709 is sound in principle. We just can't do it yet without further hobbling generated functions.)

@c42f
Copy link
Member Author

c42f commented Sep 14, 2019

The discussion at #655 has stimulated what I feel is finally a reasonable solution to constructing small static arrays with precise eltype without too much typing. Behold:

SA_F64 = SA{Float64}
SA_F32 = SA{Float32}

v = SA_F64[1,2,3]  # A Float64 vector

v = SA_F32[1 2; 3 4]  # A 2x2 Float32 SMatrix

I don't love the underscore there, but SAF64 looks very confusing.

I have implemented that and added it here with some tests.

With these I'm feeling quite good about this solution now, so I plan to merge it shortly.

Upside: This gives extremely attractive syntax for short static array
literals.

Downside: This is a syntax pun: the result of the `SA[...]` depends on
whether the name `SA` is bound to `StaticArrays.SA` or is local variable
of some other type.

However, this is no worse than Int[1,2,3], which is also a pun on the
array indexing syntax. Cases where this syntax could refer to something
other than StaticArrays.SA are detectable by the compiler and in
principle also accessible to a linter.
@EvertSchippers
Copy link
Collaborator

Thanks Chris for all your efforts! I like the ease with which a very common vector can be created like this!

@c42f
Copy link
Member Author

c42f commented Sep 15, 2019

I'll put the aliases in a separate PR so people can bikeshed the names if desired.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design speculative design related issue feature features and feature requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants