params(), Flux/Zygote style

[theogf]

I replaced all scalars by vectors so they can be mutable, and copied the Flux approach of using trainable and params to get out an array of array of parameters to be optimized. Left to be done is to figure out how to fix parameters. @devmotion I saw you helped in making a FixedKernel in GaussianProcesses.jl do you think this is the right solution?

[devmotion]

Hmm that was quite some time ago, so I don't remember anything about FixedKernel. I think the correct approach would be to follow the instruction in the documentation of Flux if you want to fix specific parameters.

[devmotion]

BTW quite disappointing that all parameters have to be vectors. I guess there's no way around that currently?

[devmotion]

Suggested change

trainable(x) = ()

IMO the implementation of trainable should be removed, since it is already defined in https://github.com/FluxML/Flux.jl/blob/a874bef6f9d8eb1ab2fe376cb8ad068eb36baf33/src/functor.jl#L40.

[devmotion]

Suggested change

	function params(k::Kernel)
	ps = []
	params!(ps,k)
	return ps
	end
	function params!(ps,k::Kernel)
	for child in trainable(k)
	params!(ps,k)
	end
	end
	params!(ps,x::AbstractArray) = push!(ps,x)

Isn't that already defined in https://github.com/FluxML/Flux.jl/blob/a874bef6f9d8eb1ab2fe376cb8ad068eb36baf33/src/functor.jl#L76-L88? It should be sufficient to define trainable for kernels that have trainable parameters.

[theogf]

BTW quite disappointing that all parameters have to be vectors. I guess there's no way around that currently?

I don't think so, Ref would be definitely a better option. But maybe at some point Flux.jl will support it and we can revert it.

[theogf]

I reimplemented Flux params to avoid the dependency, Flux being super heavy...
Do you have an alternative solution?

[devmotion]

Ah I see. Are params and trainable supposed to be used without Flux at all? If not, one could make Flux an optional dependency and define them only if Flux loaded using Requires.

[theogf]

Ah I see. Are params and trainable supposed to be used without Flux at all? If not, one could make Flux an optional dependency and define them only if Flux loaded using Requires.

Not necessarily. The problem with Requires is that the need to move all the trainable in a different file. It actually feels weird that it is not possible to do all this without importing Flux.jl that would be great that they move this in a different package.

[theogf]

Ah one easy option is to design an internal function _trainable for each kernel and with Requires to add simply Flux.trainable(k::Kernel)= _trainable(k)

[devmotion]

If the whole purpose of trainable is the integration with Flux, IMO it would be more natural to only define trainable, possibly with Requires. Since all implementations are very short, I don't see an issue with having them in a separate file.

[theogf]

Ok then that's what I did. I just need to write tests now.