Fix #30936: ElasticNet l1 ratio fails for float-only arrays #31107
Conversation
Added `_is_l1_ratio_one` function to handle l1_ratio as array, list, or tuple. Uses `np.isclose` and `np.any` to check if at least one element is approximately equal to 1.0.
There was a problem hiding this comment.
Thanks for the PR @vasco-s-pereira. I think there's a more appropriate fix, see my comment below.
| is_elasticnet_l1_penalized = "ElasticNet" in est_name and ( | ||
| (hasattr(estimator, "l1_ratio_") and np.isclose(estimator.l1_ratio_, 1.0)) | ||
| or (hasattr(estimator, "l1_ratio") and np.isclose(estimator.l1_ratio, 1.0)) | ||
| (hasattr(estimator, "l1_ratio_") and _is_l1_ratio_one(estimator.l1_ratio_)) or | ||
| (hasattr(estimator, "l1_ratio") and _is_l1_ratio_one(estimator.l1_ratio)) |
There was a problem hiding this comment.
I don't think this is the right fix. When ElasticNetCV is fitted, l1_ratio_ is a single float (the l1 ratio of the selected elastic net) and l1_ratio should be ignored. l1_ratio should only be used for ElasticNet (no CV), where it's a single float as well.
To me the right fix is more something like the following
is_elasticnet_l1_penalized = est_name == "ElasticNet" and (
(hasattr(estimator, "l1_ratio") and np.isclose(estimator.l1_ratio, 1.0))
)
is_elasticnetcv_l1_penalized = est_name == "ElasticNetCV" and (
(hasattr(estimator, "l1_ratio_") and np.isclose(estimator.l1_ratio_, 1.0))
)
if (
is_l1_penalized
or is_lasso
or is_elasticnet_l1_penalized
or is_elasticnetcv_l1_penalized
):
# the natural default threshold is 0 when l1 penalty was used
threshold = 1e-5
else:
threshold = "mean"|
Also, there are linting issues that you can fix by following the instructions here https://scikit-learn.org/dev/developers/contributing.html#how-to-contribute |
|
As for adding tests, the reproducer from the linked issue #30936 (comment) would make a good non regression test. |
|
Hi, sorry - I only saw your messages today. I'll look into it in the coming days! |
…that ElasticNetCV works with a list of floats; Add non‑regression test `test_get_feature_names_out_elasticnetcv`
There was a problem hiding this comment.
I forgot to ask you to add a changelog entry. I directly pushed it.
LGTM. Thanks @vasco-s-pereira !
|
Thank you @jeremiedbb ! |
Added
_is_l1_ratio_onefunction to handle l1_ratio as array, list, or tuple. Usesnp.iscloseandnp.anyto check if at least one element is approximately equal to 1.0.Reference Issues/PRs
Fixes #30936
What does this implement/fix? Explain your changes.
Creates a function called _is_l1_ratio_one that verifies if the l1_ratio in ElasticNet is an array, list or tuple and, in that case, utilizes
the function np.any instead of only np.isclose that only works with one value, so that it can go through every value.
Any other comments?