search

JuliaAI / MLJModelInterface.jl

Lightweight package to interface with MLJ
MIT License
37 stars 8 forks source link

Should `MLJModelInterface.metadata_pkg()` go in `__init__` or not ? #164

Closed sylvaticus closed 1 year ago

sylvaticus commented 1 year ago

In BetaML I have, for reasons I can't recall (most probably just because I have copied it from some other package), the MLJModelInterface.metadata_pkg() function inside the __init__ function, but this seems to create some problems.

Is it safe to just remove the MLJ model registration from __init__ ?

ablaom commented 1 year ago

Perhaps @tlienart would like to comment. I can't think of a reason to have that in __init__ and generally eval in __init__ is a bad idea. Most MLJ implementations do not put this in __init__.

tlienart commented 1 year ago

on the top of my head I can't think of a reason either. Looking at https://github.com/JuliaAI/MLJScikitLearnInterface.jl/blob/dev/src/MLJScikitLearnInterface.jl there is an init but it's to import sklearn, looking at https://github.com/JuliaAI/MLJDecisionTreeInterface.jl/blob/dev/src/MLJDecisionTreeInterface.jl there is a metadata_pkg in the module but no init.

The latter (DecisionTree) can probably be used as a model by all devs. Long story short I think that it should be safe to remove it from init.

https://github.com/JuliaAI/MLJDecisionTreeInterface.jl/blob/db8f8604d293719511a82e3ff5dec7d1ece8c0d7/src/MLJDecisionTreeInterface.jl#L353-L362