Add sample_weight to the calculation of alphas in enet_path and LinearModelCV #22933
Conversation
Modifies _alpha_grid function in linear_model._coordinate_descent to accept a sample_weight argument. In addition to adding this argument, I have removed the argument copy_X, which no longer seems to be needed. The function _alpha_grid is called in two places, enet_path and LinearModelCV. The new sample_weight argument is not used by enet_path, but it is used by LinearModelCV.
The old formula for alpha_max was correct, I misunderstood the way it handles Multitask regression.
Try to deal with Multitarget and single target regression
|
I have never heard of multi task regression before, so I didn't write the |
…ssian data (#20653) Co-authored-by: Olivier Grisel <olivier.grisel@gmail.com>
|
@s-banach Thank you very much for giving it a try: solving this bug as well as opening your first PR. I'll try to review as time permits. |
|
It would be a good idea to write a test in |
…purity_decrease (#22915)
Another case I didn't think about because I never use multi target regression.
…negative (#19085) Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> Co-authored-by: jeremiedbb <jeremiedbb@yahoo.fr>
I don't need to make the sparse diagonal matrix here.
I had this (1-mu) thing that I copied from somewhere but doesn't seem to make sense.
|
Dr. Lorentzen. I can try writing a test. (Or I can leave that to you, if you no longer trust my code after seeing this big mess I've made!) |
If product is left as a scipy matrix, then product**2 will not be "element-wise power"
|
@s-banach No problem at all. In the end, you need to write such a test because without it, we can't merge this PR. It is good practice (e.g. test driven development) to write such a test early. Tests are there to proof that the implementation does exactly what it is supposed to do. What I notice is that you changed to code quite a lot and I fear that this way, it might be hard to detect errors. I would suggest to make smaller changes, i.e. only introduce the additional functionality - and a test for it - nothing more. |
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
Now the logic should be unchanged from original, if sample_weight is None
|
@lorentzenchr (P.S. In the end, |
…22677) Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Jérémie du Boisberranger <jeremiedbb@users.noreply.github.com>
…#22950) * ENH support Ridge(fit_intercept=True, solver="lsqr") for sparse input * DOC add whatsnew * FIX set solver to lsqr * DOC _get_rescaled_operator * DOC add more details
…#18975) Co-authored-by: Olivier Grisel <olivier.grisel@gmail.com> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
…2663) Co-authored-by: Jérémie du Boisberranger Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Juan Gomez <jgomez75@gatech.edu>
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
* MNT replace pinvh by solve * DOC more info for svd solver * TST rewrite test_ridge * MNT remove test_ridge_singular * MNT restructure into several tests * MNT remove test_toy_ridge_object * MNT remove test_ridge_sparse_svd This is tested in test_ridge_fit_intercept_sparse_error. * TST exclude cholesky from singular problem * CLN two fixes * MNT parametrize test_ridge_sample_weights * MNT restructure test_ridge_sample_weights * CLN tighten tolerance for sag solver * CLN try to fix saga tolerance * CLN make test_ridge_sample_weights nicer * MNT remove test_ridge_regression_sample_weights * MNT rename to test_ridge_regression_sample_weights * CLN make test_ridge_regression_unpenalized pass for all random seeds * CLN make tests pass for all random seeds * DOC fix typos * TST skip cholesky for singular problems * MNT move up test_ridge_regression_sample_weights * CLN set skip reason as comment
* FIX ColumnTransformer.get_feature_names_out with string slices * Fixed black formatting * Added tests * Updated whats new 1.1 section * Simplified logic and added slices containing two elements * Fixed black linting * Linting cleanup, revert API change * Fixed linting
…ius neighborhood (#22829)
This reverts commit cc3fdbf.
* TST Adapt test_optics.py to test implementations on 32bit datasets * TST Use global_dtype * Apply review comments
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
It merely asserts that alpha_max is large enough to force the coefficients to zero. It doesn't test anything else about alpha_max or _alpha_grid.
|
Closing because I copy-pasted from your repo into my repo and now it says I'm making 66 commits. |
|
You could just restore commit c01e629, see e.g. https://stackoverflow.com/questions/4114095/how-do-i-revert-a-git-repository-to-a-previous-commit. If you do so, this would need a force push which we usually avoid - but in this situation it might be ok. I don't know which steps lead to the many commits. What we usually do is that we merge the branch main into the current PR's branch, but only if github shows merge conflicts. |
|
Thanks, I should learn more about using git. |
Reference Issues/PRs
Fixes #22914.
What does this implement/fix? Explain your changes.
Modifies
_alpha_gridfunction inlinear_model._coordinate_descentto accept asample_weightargument.In addition to adding this argument, I have removed the argument
copy_X, which no longer seems to be needed.The function
_alpha_gridis called in two places,enet_pathandLinearModelCV.The new
sample_weightargument is not used byenet_path, but it is used byLinearModelCV.Any other comments?
Thanks for your patience, this is my first PR and I'm trying my best.