Hello,
While working on a downstream package (OnlineML.jl), I noticed a discrepancy between the constructor trait documentation and the actual implementation of LearnAPI.clone.
The constructor trait documentation requires a keyword constructor and demonstrates reconstruction with:
LearnAPI.constructor(learner)(; named_properties...)
However, LearnAPI.clone currently collects the learner properties and passes them as positional arguments by splatting a NamedTuple without a semicolon:
LearnAPI.constructor(learner)(NamedTuple{names}(new_values)...)
Minimal reproducer
using LearnAPI
Base.@kwdef struct DemoLearner
rate::Float64 = 0.1
end
LearnAPI.constructor(::DemoLearner) = DemoLearner
LearnAPI.clone(DemoLearner(); rate=0.2)
Expected Behavior
DemoLearner(0.2) should be returned through the documented keyword-constructor contract (e.g., if clone splatted with a semicolon: ; NamedTuple{names}(new_values)...).
Actual Behavior
clone calls DemoLearner(0.2) positionally. A learner that intentionally provides only the documented keyword constructor (like the one generated by Base.@kwdef without a custom positional fallback) raises a MethodError.
Additional Context
Currently, downstream learners with properties must provide an additional positional constructor solely for compatibility with LearnAPI.clone.
Fixing this by changing clone to splat the named tuple as keywords (;) would align the code with the documentation. However, please note that changing clone upstream may affect existing learners that have already implemented (or only implemented) a positional constructor to work around this, so an upstream compatibility transition/deprecation phase might be necessary.
Affected code
|
return LearnAPI.constructor(learner)(NamedTuple{names}(new_values)...) |
Possible change
return LearnAPI.constructor(learner)(; NamedTuple{names}(new_values)...)
PS : this issue have been filled with AI assistance
Hello,
While working on a downstream package (OnlineML.jl), I noticed a discrepancy between the
constructortrait documentation and the actual implementation ofLearnAPI.clone.The
constructortrait documentation requires a keyword constructor and demonstrates reconstruction with:However,
LearnAPI.clonecurrently collects the learner properties and passes them as positional arguments by splatting aNamedTuplewithout a semicolon:Minimal reproducer
Expected Behavior
DemoLearner(0.2)should be returned through the documented keyword-constructor contract (e.g., ifclonesplatted with a semicolon:; NamedTuple{names}(new_values)...).Actual Behavior
clonecallsDemoLearner(0.2)positionally. A learner that intentionally provides only the documented keyword constructor (like the one generated byBase.@kwdefwithout a custom positional fallback) raises aMethodError.Additional Context
Currently, downstream learners with properties must provide an additional positional constructor solely for compatibility with
LearnAPI.clone.Fixing this by changing
cloneto splat the named tuple as keywords (;) would align the code with the documentation. However, please note that changingcloneupstream may affect existing learners that have already implemented (or only implemented) a positional constructor to work around this, so an upstream compatibility transition/deprecation phase might be necessary.Affected code
LearnAPI.jl/src/clone.jl
Line 26 in b17f7de
Possible change
PS : this issue have been filled with AI assistance