jbrea commented 2 years ago

This is a first attempt at #108.

There are quite a few docstrings that still need to be adjusted. @tlienart can you have a look at the RidgeRegression docstring for an example how I intended to adjust them?

tlienart commented 2 years ago

Apologies for the late reply, with family duties around the holidays I didn't have much time to look into this. Happy new year by the way!

This looks great, thank you very much. One small note is that we could add a function get_penalty_scale(glr, n) which would return glr.penalty * ifelse(glr.scale_penalty_with_samples, float(n), 1.0), that might be a bit less clunky in the code but it's essentially aesthetics. I can do that later.

I'm otherwise happy with this as it is, thanks!

Would you like to do any further changes or shall I merge this?

codecov-commenter commented 2 years ago

Codecov Report

Merging #109 (490561a) into dev (add2984) will decrease coverage by 0.33%. The diff coverage is 90.90%.

@@            Coverage Diff             @@
##              dev     #109      +/-   ##
==========================================
- Coverage   94.83%   94.49%   -0.34%     
==========================================
  Files          20       20              
  Lines         852      854       +2     
==========================================
- Hits          808      807       -1     
- Misses         44       47       +3

Impacted Files	Coverage Δ
src/fit/iwls.jl	`100.00% <ø> (ø)`
src/glr/constructors.jl	`100.00% <ø> (ø)`
src/mlj/classifiers.jl	`50.00% <ø> (ø)`
src/mlj/interface.jl	`80.35% <0.00%> (-4.83%)`	:arrow_down:
src/mlj/regressors.jl	`6.66% <ø> (ø)`
src/fit/analytical.jl	`100.00% <100.00%> (ø)`
src/glr/d_l2loss.jl	`100.00% <100.00%> (ø)`
src/glr/d_logistic.jl	`100.00% <100.00%> (ø)`
src/glr/d_robust.jl	`100.00% <100.00%> (ø)`
src/glr/utils.jl	`90.90% <100.00%> (-0.76%)`	:arrow_down:
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update add2984...490561a. Read the comment docs.

jbrea commented 2 years ago

Thanks for the feedback! I use a get_penalty_scale function now and fixed the docstrings.

By the way, when I started working on this PR I considered for a moment an alternative implementation with a wrapper

struct AverageLoss{L} <: Loss
    loss::L
end

(al::AverageLoss)(x::AVR, y::AVR) = al.loss(x, y)/length(y)

I found this pretty neat, but it would have required some major refactoring, mostly because of the parametric dispatch on glr for _solver, _fit etc. Are all these type restrictions really needed? Or could one get rid of them, maybe with a few traits?

But I am also fine with the current implementation. Please feel free to merge whenever you feel this PR is ready.

tlienart commented 2 years ago

I think the CI failure with Julia1 is due to MLJBase changes. The easiest is probably to just put the minimum Julia version for MLJLinearModels 0.6 to 1.5.

tlienart commented 2 years ago

Thanks a lot @jbrea for this!

With respect to AverageLoss, this sounds great and happy to help you with the refactoring if you need any help there. (I moved this suggestion to #110)

JuliaAI / MLJLinearModels.jl

scale penalty with samples #109

Codecov Report