-
Notifications
You must be signed in to change notification settings - Fork 25
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
ENH add Lasso.jl julia solver #89
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this looks super nice!! Thx @jolars
A few comments along the way.
solvers/lasso_jl.jl
Outdated
intercept=fit_intercept, | ||
randomize=false, | ||
stopearly=false, | ||
maxncoef=max(size(X, 1), size(X, 2)) * 100, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you comment on what this is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
randomize shuffles indices before cd loops (sometimes or always, I don't know), stopearly is for stopping the path (change in deviance, saturation), maxncoef is also related to stopping the path early (maximum number of nonzero coefficients). I ran into a bug in the package when these were at their defaults, but I think only the maxncoef setting is actually necessary to work around it. I'll experiment a bit. randomize should really be true I think, but the others do not really matter.
For the broken test, we need to skip the julia solver for |
Co-authored-by: Thomas Moreau <[email protected]>
Co-authored-by: Thomas Moreau <[email protected]>
…to lassojl-solver
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! thx @jolars
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mathurinm merge if happy
We talked about this a bit in benchopt/benchopt#372. I actually think the warning might be a bug: JuliaPy/pyjulia#497 |
So we silence the warning in the solver for now ? |
my setup is segfaulting for some reason right now, but I submitted a patch to ignore the warnings somewhat blindly. Can anybody please check that it works? |
can you send me the command that causes the segfault ? |
I just tried to call |
It works but:
|
that sounds like JIT compiliation
Hm, strange. I can silence the same warning in a small script just by doing the same thing: from julia import Julia
import warnings
warnings.filterwarnings("ignore", category=FutureWarning)
jl = Julia(compiled_modules=False)
jl.eval("1 + 1")
What kind of errors? I used to receive errors previously due to convergence issues but I thought I had fixed them with a try catch statement. Are you sure that you don't have results cached or something? |
There seems to be something wrong with sparse datasets:
It's because sparse datasets are skipped by benchopt for julia, I remove this in |
I am getting the following error when trying to run this now:
I am guessing that this has something to do with the mismatch in versions between conda and whatever this other python 3.9.10 version is doing there |
Ah, because it behaves like glmnet and returns a vector of 0s if it does not converge for the desired lambda ? |
This PR adds support for the Lasso.jl Julia package. One problem with the solver is that it has no setting for maximum number of iterations, so when tolerance is low it may fail to converge. I've filed an issue at the repo about this and plan to file a PR eventually to get this fixed.