-
Notifications
You must be signed in to change notification settings - Fork 90
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
[BUG]: [MLJ Interface] SRRegressor
likely has too broad a target_scitype
#390
Comments
This is technically correct because struct Force{T}
x::T
y::T
z::T
end Maybe I should try to define the |
What about this? MMI.metadata_model(
SRRegressor;
input_scitype,
target_scitype=AbstractVector{<:Any}
supports_weights=true,
reports_feature_importances=false,
load_path="SymbolicRegression.MLJInterfaceModule.SRRegressor",
human_name="Symbolic Regression via Evolutionary Search",
)
# ^ Existing code
# For standard `Expression` type, we require continuous input
MMI.target_scitype(::Type{<:SRRegressor{<:Any,<:Any,Expression}}}) = AbstractVector{<:Continuous} |
The searchable MLJ model registry basically requires that traits parameter-independent, so your suggestion doesn't work. This isn't really something we can change, because encoding parameter-dependent traits in the registry would be a big complication. The only thing that gets written in the registry is the trait applied to the union type. (See also here, from another project.) The main suggestions I can think of are:
Can you make one of these work? You could alternatively split your model into two different models - one for regular targets, and one for unorthodox targets, but that still leaves you with the problem of specifying a loss fallback for the unorthodox model. |
Thanks, makes sense to me. Questions:
I don't think there's a way to have a dummy loss function here. When I say you can regress anything, I really do mean anything – floats, bools, tuples, named tuples, structs, hierarchical structs, images, symbols, strings, pointers to complicated C++ objects, etc., etc. You just need to pass whatever It's also the case that a user can define a custom type of |
Yes, you can make parameter-dependent
You could. You would have to issue a warning telling the user you're changing a global setting. This sounds like a drastic hack to me.
Yeah, it's good if things work out of the box. (And out-of-the-box functionality is needed if your model is to remain part of MLJ integration tests.)
Yes, using the more restrictive target scitype in the registry shouldn't prevent the user from getting it to work (by changing the scitype_warning level or somehow adding appropriate parameter-specializations of
Okay. I can't imagine common use cases for this kind of thing, but I'm happy to take your word for it. What about a misclassification rate, in the case the data is not |
Ok, I think the best is probably just this: target_scitype(::Type{<:SRRegressor}) = AbstractVector{Continuous}
target_scitype(::Type{<:SRRegressor{<:Any,<:Any,TemplateExpression}) = AbstractVector{<:Any} So it appears in the registry while also not flagging people using TemplateExpression at runtime. I'll push a fix. I think people defining their own expression types are probably comfortable enough getting their hands dirty that they wouldn't be scared off by mismatched scitype warnings anyways, so I'll avoid trying to include other expression types. Is there something other than |
Let me know if you're happy with #392 |
Looks like a good plan. I can look at #392 tomorrow or the next day.
If you declare Here is the detailed logic:
|
Cool! Ok I will do that for the TemplateExpression case, and move back to |
What happened?
The
target_scitype
is probably meant to returnAbstractVector{Continuous}
or maybeAbstractVector{<:Infinite}
but the current setting is very permissive:For consider this example:
Version
1.5.0
Operating System
macOS
Interface
Julia REPL
Relevant log output
Extra Info
No response
The text was updated successfully, but these errors were encountered: