Flux.jl: flip() calling reverse() without dimensions as keyword argument

[thebhatman]

In recurrent.jl, https://github.com/FluxML/Flux.jl/blob/22cb732657f127416822b86b752fa06bb5b2c283/src/layers/recurrent.jl#L72 , flip is calling reverse from abstractarraymath.jl . But reverse requires dims as an argument, which is not provided.

[MikeInnes]

julia> reverse([1, 2, 3])
3-element Array{Int64,1}:
 3
 2
 1

Seems fine to me; do you have an example that errors?

[thebhatman]

I wanted to use flip (defined in recurrent.jl) for a 2d tensor. Had a discussion about this on slack. The present implementation of flip works for 1d arrays and tuples where specifying dimensions is not required.

[MikeInnes]

I see. So we could just forward kwargs from flip to reverse?

[thebhatman]

Yes. That's what I had in mind.

[mcabbott]

I think we discussed this on slack, and in this example flip was applied to a tuple not an array. And f is a layer containing tracked parameters, which is why the order of evaluation of f.(x) might matter for the gradient, if I understand right.

The PR seems to make a different function, in which f is not broadcasted. That might be useful but perhaps it shouldn't share the name?

[MikeInnes]

Yeah, the PR should not change the semantics; if you need that then it's something you'll have to write yourself.

Also, dims should be a keyword argument for consistency; we can avoid needing a default argument by just splatting all keyword arguments.

[thebhatman]

Yeah, I actually wanted f(x) and not f.(x), because I wanted f to be an LSTM layer. Okay I will change it to be f.(x)

[mcabbott]

But is there a need for the f.(x) version acting on an array? For this to matter you would need (if I understand right) a multi-dimensional array of tracked arrays. (Or a scalar "layer" containing some tracked parameters.)

Possibly defining it as flip(f, xs::Tuple) would better indicate what this is for, though.

[thebhatman]

Okay I guess this is a use- specific issue and if users want to use flip over multi-dimensional arrays, they can write their own function. So I am closing the PR as f.(x) for array isn't of much use.