Loosen signature for repeat()

[nalimilan]

Put 'outer' first as it is more natural to omit 'inner'. Deprecate the old version with keyword arguments.
Also change the signature to accept any AbstractArray as first argument, and any type for 'outer' and 'inner', to allow in particular passing tuples or arrays. Allow passing an empty collection to imply a no-op.

Fixes #12953.

I suspect I should update the docs after moving them out of helpdb.jl, but I don't know the command to run.

[tkelman]

adjust the rst signature to be consistent with your change, then run ./julia doc/genstdlib.jl

[tkelman]

these should probably be doctests

[nalimilan]

Right.

[tkelman]

I personally think keyword arguments are clearer than positional. Is this working around an inference limitation that will be fixed soonish?

[nalimilan]

adjust the rst signature to be consistent with your change, then run ./julia doc/genstdlib.jl

Thanks, but I can't get it to work. Should the signature be the one in the code or in the doc? (Either way, it doesn't work...)

Regarding keyword vs. positional arguments, @simonster noted on the issue that inference would be possible, but I've not been able to get it to work for now.

[nalimilan]

I managed to get the docs to work, I hadn't rebuilt Julia correctly before updating them. I've also added doctests.

[nalimilan]

Can somebody comment about the possibility to infer the return type, now or in the future? Anybody else opposed to using positional arguments (which appeared to be the consensus on #12953 -- but I'm fine with either)?

[timholy]

The key is to infer the length of the created array's size tuple:

function rpt{N}(A::AbstractArray, outer::NTuple{N,Int})
    sz = rptsz(A, outer)
    Arpt = Array(eltype(A), sz)
    ...
end

This is pretty easy to write using generated functions, because in a generated function you can do things like M<N where M and N are two integer type parameters. However, various folks have advanced arguments that we should be doing more without generated functions, so in that spirit:

rptsz(A, ::Tuple{}) = size(A)
rptsz(A, outer::Tuple) = fill_length((), _rptsz((), A, outer), size(A))
# Build up the sz tuple so that we've consumed all elements of outer
@inline _rptsz{N}(sz::NTuple{N}, A::AbstractArray, ::Tuple{}) = sz
@inline _rptsz{N}(sz::NTuple{N}, A::AbstractArray, outer) = _rptsz((sz..., outer[1]*size(A,N+1)), A, Base.tail(outer))
# Take the preferred tuple, filling in with elements of default if it is longer
@inline fill_length(out, preferred, default) = fill_length((out..., preferred[1]), Base.tail(preferred), Base.tail(default))
@inline fill_length(out, ::Tuple{}, ::Tuple{}) = out
@inline fill_length(out, preferred, ::Tuple{}) = (out..., preferred...)
@inline fill_length(out, ::Tuple{}, default) = (out..., default...)

All of those @inlines guarantee that this nested set of function calls will be resolved at compile time.

Demo:

julia> using Base.Test

julia> @inferred(rptsz(zeros(2,2), ()))
(2,2)

julia> @inferred(rptsz(zeros(2,2), (5,)))
(10,2)

julia> @inferred(rptsz(zeros(2,2), (5,3)))
(10,6)

julia> @inferred(rptsz(zeros(2,2), (5,3,2)))
(10,6,2)

[timholy]

In case it's not obvious from the code, the way this works is by progressively peeling off the first element of corresponding tuples. You start with out = () and preferred = (10,6,2) and one-by-one append the first element of preferred to out.

[nalimilan]

Thanks, Tim! So it looks like moving to positional arguments is worth it. Is everybody OK with merging this?

[timholy]

To make sure it's clear to everyone, this version produces a result that's still not inferrable, but it will be easier to get there. If you'd like, we could merge this and then I can polish up the code above.

[nalimilan]

Fine with me. Will merge soon.

[tkelman]

That doesn't really answer the question of whether the same performance would eventually be achievable with keyword arguments, does it? This seems like a step backwards on the API.

[nalimilan]

The broader issue about whether keyword arguments will be taken into account for type inference is indeed crucial here. All I can say is that in the past several functions were changed to use positional arguments for performance, but I would be very happy if this wasn't required anymore.

[timholy]

Keyword arguments interact with performance in two distinct ways:

• Call performance This is important for functions that execute very quickly. For example, you wouldn't want to use keyword arguments with getindex.
• Inferrability

I think for most uses of repeat, the first is irrelevant. Are there precedents for changing solely for the 2nd reason? EDIT: keep in mind that one can always use a function barrier on the results of repeat.

[simonster]

pivot used to be a keyword argument for cholfact/qrfact but it's now a positional argument (Val{true}/Val{false}, which is even more of a hack) for return type inference.

[nalimilan]

That said, performance for repeat isn't critical right now, so if we're sure type inference will work with keyword arguments will come at some point, it would be perfectly reasonable to leave it as-is.

[simonster]

The problem is that it's not performance for repeat, it's performance for tight loops involving the array returned by repeat. I'm not convinced that doesn't matter.

[nalimilan]

@JeffBezanson Any thoughts about this?

[JeffBezanson]

I think we should keep the API the way we want it, and improve the compiler.

[nalimilan]

OK, I'll update the PR to only loosen the signature.

[nalimilan]

I've updated the PR to only loosen the signature, keeping keyword arguments. I leave type-inferrability for another PR -- let me know if you think it can be achieved (and how).

[tkelman]

() doesn't seem to accurately reflect the real default?

[nalimilan]

Yeah, the old code used a similar "trick". The actual value ones(Int, ndims(A)) is a bit long, though not the end of the world. I've also tried another solution in which passing an empty collection is supported and is internally replaced with a vector of ones. Which one would you prefer?

[tkelman]

Not sure which implementation should be preferred. In terms of making the default in the docs match the more complicated version, how about giving the default value some temporary variable name that gets explained in more detail in the docstring body?

[nalimilan]

I don't think the additional complexity for the reader is worth it. ones(Int, ndims(A)) is almost as clear.

[nalimilan]

Other opinions?

[nalimilan]

OK, I've put the actual signature in the docs, that's the most obvious solution. Should be good to go now.

[nalimilan]

No objections against merging?