jbrea / MLJGLMNetInterface.jl

Other
0 stars 0 forks source link

[Review] Of this new MLJ interface package #2

Open ablaom opened 3 years ago

ablaom commented 3 years ago

@jbrea Thanks very much indeed for this fine contribution. It is clear you have invested considerable time wrapping your head around the MLJ model API.

My main comment on the interface is that in MLJ it is conventional to flatten out the various parameters in the structs, rather than bundle them into a single kwarg as you have here.

So,

mutable struct MyModel
   alpha::A
   beta::B
   gamma::C
end

rather than

mutable struct MyModel{K}
    kwargs::K
end

You could leave the struct as is, but then you would have to overload Base.getproperty/Base.properties/Base.setproperty! to make it appear as if the fields are flattened out. We have done this for composite model types, where the number of fields (component models) is variable, but it hardly seems worth your while here. Is there a reason for this?

It would be nice to implement clean! methods to do sanity checks on the parameters.

Otherwise, this looks good to go.

Next steps:

ablaom commented 2 years ago

@jbrea Wondering if you were still interested in pursuing this. As I recall, this looked pretty near ready to roll.