inconsistency between params and destructure

[CarloLucibello]

In my understanding, destructure is supposed to behave similarly to params and collect trainable params of a struct into a vector. Instead, it collects all arrays, as one can see in this example using BatchNorm:

julia> b = BatchNorm(2)
BatchNorm(2)        # 4 parameters, plus 4 non-trainable

julia> ps = params(b)
Params([Float32[0.0, 0.0], Float32[1.0, 1.0]])

julia> p, re = Flux.destructure(b)
(Float32[0.0, 0.0, 1.0, 1.0, 0.0, 0.0, 1.0, 1.0], Flux.var"#60#62"{BatchNorm{typeof(identity), Vector{Float32}, Float32, Vector{Float32}}}(BatchNorm(2)))

julia> length(p)
8

Should we modify destructure to act like params, therefore recurse over trainable(m) instead of just applying fmap as it currently does?

Also, maybe we should add to both params and destructure a keyword argument which with possible values :trainable, :buffer, :all, and default value :trainable.

Related to #1727

[DhairyaLGandhi]

They're pretty different functions and meant for different reasons. destructred outputs can be used by external libraries, which conservatively need all the parameters from a training routine to be used downstream.

[CarloLucibello]

Seems exactly the same use to me, training, which means that both functions should output what is meant to be trained. I think we need some input from SciML people, since they are prominent consumers of desctructure @ChrisRackauckas

[DhairyaLGandhi]

Not just training, some libraries use it for initial guesses as well. I'd prefer to not make trainable a "special" requirement for defining layers

[ToucheSir]

Differing use cases aside, making destructure call trainable won't make trainable any more of a requirement for defining layers than it already is (i.e. not at all), because it forwards to functor by default.

[ChrisRackauckas]

I thought it only grabbed the trainable params 😅 . The reason is to, for example, get the vector and use BFGS to train the parameters of the Chain. If it's not giving the same vector out, then training with Optim wouldn't be the same as training with Flux's optimizers, which is not what we would intend. So it should probably exclude the values that are meant to be constant.

[darsnack]

We can switch to using the new walk keyword for fmap to only walk the trainable parameters with an option to pass an alternate walk into destructure.

[DhairyaLGandhi]

It currently excluded some values that we would want included at higher order cases, and on our end it's actually easier to ignore the objects that don't have gradients which can differ depending on layer configurations by grabbing the gradients from the NamedTuple directly. That is a nice and generic approach.