Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add AutoEncoderMLJ model (part of BetaML) #1074

Closed
sylvaticus opened this issue Dec 29, 2023 · 10 comments
Closed

Add AutoEncoderMLJ model (part of BetaML) #1074

sylvaticus opened this issue Dec 29, 2023 · 10 comments

Comments

@sylvaticus
Copy link
Contributor

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 :-)

@ablaom
Copy link
Member

ablaom commented Jan 3, 2024

Thanks @sylvaticus for adding the wrapper. It's mostly good.

Main thing is that

mutable struct AutoEncoderMLJ <: MLJModelInterface.Deterministic

should be

mutable struct AutoEncoderMLJ <: MLJModelInterface.Unsupervised

Having "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

@ablaom
Copy link
Member

ablaom commented Jan 10, 2024

FYI I have just updated the MLJ Model Registry which has automatically added this model - my mistake. I don't think that's a big deal, but do let me know as soon as you have fixed Deterministic -> Unsupervised so I can update the registry again.

@sylvaticus
Copy link
Contributor Author

sylvaticus commented Jan 10, 2024 via email

@sylvaticus
Copy link
Contributor Author

I have fixed the Deterministic -> Unsupervised issue and changed its name to AutoEncoder by setting the MLJ in its own submodule BetaML.Bmlj.

MLJTestInterface.test passes, but I am still unsure about MLJModelInterface.metadata_pkg.. there (at the end of /src/BetaML.jl file) I use Bmlj.AutoEncoder, is it fine like that ?

@ablaom
Copy link
Member

ablaom commented Jan 14, 2024

MLJTestInterface.test passes, but I am still unsure about MLJModelInterface.metadata_pkg.. there (at the end of /src/BetaML.jl file) I use Bmlj.AutoEncoder, is it fine like that ?

I'm not completely sure I understand the question, but I do see that the package-related metadata is getting defined for Bmlj.AutoEncoder:

julia> package_name(BetaML.Bmlj.AutoEncoder)
"BetaML"

Is that what you were worried about?

@sylvaticus
Copy link
Contributor Author

Yes, is it fine like that?

@ablaom
Copy link
Member

ablaom commented Jan 14, 2024

Yes. It looks good to me. The most important thing is that the load path is good and it is:

julia> load_path(BetaML.Bmlj.AutoEncoder)
"BetaML.Bmlj.AutoEncoder"

@ablaom
Copy link
Member

ablaom commented Jan 16, 2024

Note to self: This issue is addressed on BetaML#master; waiting for next release (currently at 0.10.4).

@sylvaticus
Copy link
Contributor Author

sylvaticus commented Jan 25, 2024

just done it. Released v0.11.0 with all MLJ interface models isolated in their own submodule. This allowed the MLJ model struct to share the same name with BetaML models and I have taken the occasion to uniform all the model names.. (so, unfortunately, big breaking release)..

@ablaom
Copy link
Member

ablaom commented Jan 25, 2024

Congratulations on the refactor.

This is issue can be closed after:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants