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

Allow @SVector comprehension to eval range with locals? #26

Closed
dbeach24 opened this issue Sep 5, 2016 · 11 comments
Closed

Allow @SVector comprehension to eval range with locals? #26

dbeach24 opened this issue Sep 5, 2016 · 11 comments
Labels
feature features and feature requests

Comments

@dbeach24
Copy link

dbeach24 commented Sep 5, 2016

Because the @SVector macro does not evaluate the range using the local scope, I find myself transforming code like this:

function mytransform{N}(x::SVector{N})
    @SVector [x[i] * i for i=1:N]
end

into this:

function mytransform{N,T}(x::SVector{N,T})
    SVector{N,T}(ntuple(i -> x[i] * i, N) ...)
end

I'm not sure if the second form is as efficient, but in either case, the first version is far easier to read.

Is there any chance that @SVector comprehensions can be made to support local variables in the range? Or, is there some other way I should be writing this?

@c42f
Copy link
Member

c42f commented Sep 5, 2016

Normally @SVector unrolls the generator expression, but that's not possible in this case without extra compiler support since N isn't a literal known to the macro.

However, the second version can be written directly with a generator which isn't too bad:

function mytransform{N,T}(x::SVector{N,T})
    SVector{N,T}(x[i]*i for i=1:N)
end

From a quick look at code_native, it seems that a call is still being made so this probably won't be super fast. It may be possible to improve the codegen with an extra generated function, to unroll once N is known?

@andyferris
Copy link
Member

@dbeach24 This is something I'd like to do but as @c42f mentions its a little difficult with current compiler support. I have an idea for one hack which would end up using a call and a loop yet AFAIK it still should be faster to write

function mytransform{N,T}(x::SVector{N,T})
    @inbounds return SVector{N,T}(x[i]*i for i=1:N)
end

as @c42f we need an @inbounds somewhere for the x[i] (otherwise I don't know why it would call, but if that doesn't work then I should investigate).

Another approach is to have a statically valued thing like SUnitRange{1, N}() <: StaticVector{Int} and call map on it. This will probably be implemented sometime soonish.

To my understanding, this one:

function mytransform{N,T}(x::SVector{N,T})
    SVector{N,T}(ntuple(i -> x[i] * i, N) ...)
end

should be performant up until N = 16 and slow afterwards, The compiler people are interested in making this fast for all N (a longer term goal, however).

@andyferris
Copy link
Member

I've started to look at this in #30

@andyferris
Copy link
Member

andyferris commented Nov 14, 2016

OK I've had a thought. First we implement function initialization for all StaticArrays such that

SVector{N}(f::Function)

populates SVector{N}((f(1), f(2), ..., f(N)) (and so-on for higher dimensional arrays). This would be useful in its own right.

Furthermore, transform this:

@SVector [f(i) for i = inds]

into this

N = size(inds) # can we check/enforce if this is statically determinable? E.g. size(typeof(inds)) is OK.
SVector{N}(j -> f(inds[i]))

We can assume the inds collection is an indexable collection, since it's useless (speed wise) if its size is unknown...

It would help syntactically if we made colon(a,b) into a @pure function, as well as size(::UnitRange).

@andyferris
Copy link
Member

andyferris commented Nov 14, 2016

BTW @dbeach24 the ... in the below is unnecessary (in fact, a tuple is desirable) and you should use ntuple(f, Val{N}).

function mytransform{N,T}(x::SVector{N,T})
    SVector{N,T}(ntuple(i -> x[i] * i, N) ...)
end

So this is fast (up to N = 15 or so)

function mytransform{N,T}(x::SVector{N,T})
    SVector{N,T}(ntuple(i -> x[i] * i, Val{N}))
end

@ChristianKurz
Copy link

The construct in question

function mytransform{N}(x::SVector{N})
    @SVector [x[i] * i for i=1:N]
end

can be written without the macro like this:

function mytransform{N}(x::SVector{N})
    SVector([x[i] * i for i=1:N]...)
end

Though i feel like the SVector()-constructor should be able to handle arrays without the splatting.

@KristofferC
Copy link
Contributor

KristofferC commented Jan 5, 2017

That would allocate a temporary array as well as causing the output type to not be inferrable.

@garrison
Copy link
Contributor

Slightly OT: In two comments above it is mentioned that the following workaround should work

function mytransform{N,T}(x::SVector{N,T})
    SVector{N,T}(x[i]*i for i=1:N)
end

however, when I call it as mytransform(@SVector [1,2,3]), I get ERROR: No precise constructor for SVector{3,Int64} found. Length of input was 1.

Did passing a generator to the SVector constructor in this way work in a previous version of StaticArrays? (Should this be considered a regression?)

@andyferris
Copy link
Member

We've never robustly supported generators in the past. It's something I'd like to improve on, though. Hmm...

@c42f
Copy link
Member

c42f commented Aug 1, 2019

I think this issue is basically the same as #97

@c42f c42f closed this as completed Aug 1, 2019
@c42f c42f added the feature features and feature requests label Aug 1, 2019
@c42f c42f mentioned this issue Oct 22, 2019
@kbarros
Copy link

kbarros commented Jan 4, 2023

Note that SVector{3}(f(x) for x in xs) works as of #792. See also: #97 (comment).

oschulz pushed a commit to oschulz/StaticArrays.jl that referenced this issue Apr 4, 2023
* add pair constructor

* add StructVector

* added empty and indexstyle

* export StructVector

* add columns colnames and ncols

* test columns

* test empty
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature features and feature requests
Projects
None yet
Development

No branches or pull requests

7 participants