search

JuliaAI / MLJLinearModels.jl

Generalized Linear Regressions Models (penalized regressions, robust regressions, ...)
MIT License
81 stars 13 forks source link

`LogisticClassifier` lambda and gamma are easy to confuse #122

Closed rikhuijzer closed 2 years ago

rikhuijzer commented 2 years ago

Currently lambda is the strength of L1 if :l1, L2 if :l2 and L2 in :en which is very tricky. It is easy to mistake them when switching the penalty. Should we refactor this to

with_kw_noshow mutable struct LogisticClassifier <: MMI.Probabilistic
    "strength of the l1 regulariser if `penalty` is `:l1` or `:en`"
    l1::Real = 1.0
    "strength of the l2 regularizer if `penalty` is `:l2` or `:en`"
    l2::Real = 1.0
[...]
tlienart commented 2 years ago

I'm personally against this. The PR #121 you made for the internal docstring makes sense to me but I don't think we should go beyond that. The naming of symbol here goes with the literature I was working with and helps uniformity (it's fairly standard though, of course, there may be other schemes out there).

We can always make the docstring for the constructors more legible but given it's extremely common to have lambda used for both the l1 and l2 problem, I don't see a problem doing the same when there's a single regulariser so that it's clear what it affects.