ablaom
commented
10 months ago Thanks @sylvaticus for adding the wrapper. It's mostly good.
Main thing is that
mutable struct AutoEncoderMLJ <: MLJModelInterface.Deterministicshould be
mutable struct AutoEncoderMLJ <: MLJModelInterface.UnsupervisedHaving "MLJ" in the name is unfortunate. Perhaps you put the interface in a submodule to prevent name conflict with AutoEncoder? If you do that, you need to need to amend the load path at https://github.com/sylvaticus/BetaML.jl/blob/38074a9207ba36bd3427bc31eef42f7b990d1ac8/src/Utils/Utils_MLJ.jl#L159
The doc string is great but not strictly compliant. There are examples for transformers (Unsupervised models) at the end of this page.
There's no need to implement predict for Unsupervised models.
Feel free to add the following interface test, but that's up to you:
import BetaML
using MLJTestInterface
@testset "generic mlj interface test" begin
fails, summary = MLJTestInterface.test(
[BetaML.Utils.AutoEncoderMLJ,],
MLJTestInterface.make_regression()[1];
mod=@__MODULE__,
verbosity=0, # bump to debug
throw=false, # set to true to debug
)
@test isempty(failures)
end
sylvaticus
Hello, I have added to BetaML a model to use a AutoEncoder in a very simple way from the end user prospective, without the need to understand the underlying neural network. I have also created a MLJ wrapper,
AutoEncoderMLJ. As this is my first transformer for MLJ, could you please check that the wrapper is fine and eventually add it to the MLJ registry ?Thank you :-)