Repository: scikit-learn/scikit-learn
Pull request: FIX fix comparison between array-like parameters when detecting non-default params for HTML representation
<!--
Thanks for contributing a pull request! Please ensure you have taken a look at
the contribution guidelines: https://github.com/scikit-learn/scikit-learn/blob/main/CONTRIBUTING.md
-->

#### Reference Issues/PRs
<!--
Example: Fixes #1234. See also #3456.
Please use keywords (e.g., Fixes) to create link to the issues or pull requests
you resolved, so that they will automatically be closed when your pull request
is merged. See https://github.com/blog/1506-closing-issues-via-pull-requests
-->
Fixes #31525

#### What does this implement/fix? Explain your changes.
As @glemaitre commented, we were comparing 2 different-size arrays:
 `param_value != init_default_params[param_name]`


#### Any other comments?
😱

<!--
Please be aware that we are a loose team of volunteers so patience is
necessary; assistance handling other issues is very welcome. We value
all user contributions, no matter how minor they are. If we are slow to
review, either the pull request needs some benchmarking, tinkering,
convincing, etc. or more likely the reviewers are simply busy. In either
case, we ask for your understanding during the review process.
For more information, see our FAQ on this topic:
https://scikit-learn.org/dev/faq.html#why-is-my-pull-request-not-getting-any-attention.

Thanks for contributing!
-->


Problem statement:
Issue with the `RidgeCV` diagram representation with non-default alphas
It seems that we introduced a regression in the HTML representation. The following code is failing:

```python
import numpy as np
from sklearn.linear_model import RidgeCV

RidgeCV(np.logspace(-3, 3, num=10)
```

leads to the following error:

```pytb
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
File ~/Documents/teaching/demo_data_science_agent/.pixi/envs/default/lib/python3.13/site-packages/IPython/core/formatters.py:406, in BaseFormatter.__call__(self, obj)
    404     method = get_real_method(obj, self.print_method)
    405     if method is not None:
--> 406         return method()
    407     return None
    408 else:

File ~/Documents/teaching/demo_data_science_agent/.pixi/envs/default/lib/python3.13/site-packages/sklearn/utils/_repr_html/base.py:145, in ReprHTMLMixin._repr_html_inner(self)
    140 def _repr_html_inner(self):
    141     """This function is returned by the @property `_repr_html_` to make
    142     `hasattr(estimator, "_repr_html_") return `True` or `False` depending
    143     on `get_config()["display"]`.
    144     """
--> 145     return self._html_repr()

File ~/Documents/teaching/demo_data_science_agent/.pixi/envs/default/lib/python3.13/site-packages/sklearn/utils/_repr_html/estimator.py:480, in estimator_html_repr(estimator)
    469 html_template = (
    470     f"<style>{style_with_id}</style>"
    471     f"<body>"
   (...)    476     '<div class="sk-container" hidden>'
    477 )
    479 out.write(html_template)
--> 480 _write_estimator_html(
    481     out,
    482     estimator,
    483     estimator.__class__.__name__,
    484     estimator_str,
    485     first_call=True,
    486     is_fitted_css_class=is_fitted_css_class,
    487     is_fitted_icon=is_fitted_icon,
    488 )
    489 with open(str(Path(__file__).parent / "estimator.js"), "r") as f:
    490     script = f.read()

File ~/Documents/teaching/demo_data_science_agent/.pixi/envs/default/lib/python3.13/site-packages/sklearn/utils/_repr_html/estimator.py:386, in _write_estimator_html(out, estimator, estimator_label, estimator_label_details, is_fitted_css_class, is_fitted_icon, first_call, param_prefix)
    384 elif est_block.kind == "single":
    385     if hasattr(estimator, "_get_params_html"):
--> 386         params = estimator._get_params_html()._repr_html_inner()
    387     else:
    388         params = ""

File ~/Documents/teaching/demo_data_science_agent/.pixi/envs/default/lib/python3.13/site-packages/sklearn/base.py:310, in BaseEstimator._get_params_html(self, deep)
    306 ordered_out = {name: out[name] for name in init_default_params if name in out}
    307 ordered_out.update({name: out[name] for name in remaining_params})
    309 non_default_ls = tuple(
--> 310     [name for name, value in ordered_out.items() if is_non_default(name, value)]
    311 )
    313 return ParamsDict(ordered_out, non_default=non_default_ls)

File ~/Documents/teaching/demo_data_science_agent/.pixi/envs/default/lib/python3.13/site-packages/sklearn/base.py:296, in BaseEstimator._get_params_html.<locals>.is_non_default(param_name, param_value)
    291 if isinstance(param_value, BaseEstimator) and type(param_value) is not type(
    292     init_default_params[param_name]
    293 ):
    294     return True
--> 296 if param_value != init_default_params[param_name] and not (
    297     is_scalar_nan(init_default_params[param_name])
    298     and is_scalar_nan(param_value)
    299 ):
    300     return True
    301 return False

ValueError: operands could not be broadcast together with shapes (10,) (3,) 
```

@DeaMariaLeon Would you mind to look at it. If I get the time, I can look at it a bit more in details.

Hints from issue discussion:
My guess here is that `param_value != init_default_params[param_name]` compare 2 numpy arrays:

```python
np.array([1, 2, 3]) != np.array([1, 2, 3, 4])
```

```pytb
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
Cell In[10], line 1
----> 1 np.array([1, 2, 3]) != np.array([1, 2, 3, 4])

ValueError: operands could not be broadcast together with shapes (3,) (4,) 
```

It happens because the two arrays do not have the same size. I'm thinking that we could safely have a `try`/`except` around the comparison because if it fails, it means that the parameter are not the same:

```python
try:
    if param_value != init_default_params[param_name] and not (
           is_scalar_nan(init_default_params[param_name])
           and is_scalar_nan(param_value)
    ):
        return True
except Exception:
    # any exception raised means that we don't have the same
    # parameters
    return True
return False
```

Something in this line of spirit.

Commit message: added array vs scalar

Patch to review:
diff --git a/doc/whats_new/upcoming_changes/sklearn.base/31528.fix.rst b/doc/whats_new/upcoming_changes/sklearn.base/31528.fix.rst
new file mode 100644
index 0000000000000..25bf0404c67b2
--- /dev/null
+++ b/doc/whats_new/upcoming_changes/sklearn.base/31528.fix.rst
@@ -0,0 +1,3 @@
+- Fix regression on the :class: `linear_model.RidgeCV` diagram representation
+  with non-default alphas.
+  By :user:`Dea María Léon <deamarialeon>`
diff --git a/sklearn/base.py b/sklearn/base.py
index 309b482357e12..e9308d8f1376f 100644
--- a/sklearn/base.py
+++ b/sklearn/base.py
@@ -292,12 +292,14 @@ def is_non_default(param_name, param_value):
                 init_default_params[param_name]
             ):
                 return True
-
-            if param_value != init_default_params[param_name] and not (
+            if not np.array_equal(
+                param_value, init_default_params[param_name]
+            ) and not (
                 is_scalar_nan(init_default_params[param_name])
                 and is_scalar_nan(param_value)
             ):
                 return True
+
             return False
 
         # reorder the parameters from `self.get_params` using the `__init__`
diff --git a/sklearn/tests/test_base.py b/sklearn/tests/test_base.py
index e57d36351f0d4..41a5ced6a30d1 100644
--- a/sklearn/tests/test_base.py
+++ b/sklearn/tests/test_base.py
@@ -26,6 +26,7 @@
 from sklearn.decomposition import PCA
 from sklearn.ensemble import IsolationForest
 from sklearn.exceptions import InconsistentVersionWarning
+from sklearn.linear_model import RidgeCV
 from sklearn.model_selection import GridSearchCV
 from sklearn.pipeline import Pipeline
 from sklearn.preprocessing import StandardScaler
@@ -1000,3 +1001,46 @@ def test_get_params_html():
 
     assert est._get_params_html() == {"l1": 0, "empty": "test"}
     assert est._get_params_html().non_default == ("empty",)
+
+
+# non regression test see github issue #31525
+def test_get_params_html_ridgecv():
+    est = RidgeCV(np.logspace(-3, 3, num=10))
+    assert_allclose(
+        est._get_params_html()["alphas"],
+        np.array(
+            [
+                1.00000000e-03,
+                4.64158883e-03,
+                2.15443469e-02,
+                1.00000000e-01,
+                4.64158883e-01,
+                2.15443469e00,
+                1.00000000e01,
+                4.64158883e01,
+                2.15443469e02,
+                1.00000000e03,
+            ]
+        ),
+    )
+
+
+@pytest.mark.parametrize(
+    "initial_value, instance_value, non_default_length",
+    [
+        (np.array([np.float32(2)]), [2], 0),
+        ([2.0], [np.int32(2)], 0),
+        (True, 1, 0),
+        ([1, 2], [3], 1),
+        (np.array([1]), 1, 1),
+    ],
+)
+def test_get_params_html_types(initial_value, instance_value, non_default_length):
+    class MyEstimator(BaseEstimator):
+        def __init__(self, initial_value=initial_value):
+            self.initial_value = initial_value
+
+    est = MyEstimator(initial_value=instance_value)
+    # when initial_value and instance_value are different, it means
+    # that instance_value is a non default value
+    assert len(est._get_params_html().non_default) == non_default_length


Code review comments:
diff_hunk: @@ -1000,3 +1001,46 @@ def test_get_params_html():
 
     assert est._get_params_html() == {"l1": 0, "empty": "test"}
     assert est._get_params_html().non_default == ("empty",)
+
+
+# non regression test see github issue #31525
+def test_get_params_html_ridgecv():
+    est = RidgeCV(np.logspace(-3, 3, num=10))
+    assert_allclose(
+        est._get_params_html()["alphas"],
+        np.array(
+            [
+                1.00000000e-03,
+                4.64158883e-03,
+                2.15443469e-02,
+                1.00000000e-01,
+                4.64158883e-01,
+                2.15443469e00,
+                1.00000000e01,
+                4.64158883e01,
+                2.15443469e02,
+                1.00000000e03,
+            ]
+        ),
+    )
+
+
+@pytest.mark.parametrize(
+    "initial_value, instance_value, non_default_length",
+    [
+        (np.array([np.float32(2)]), [2], 0),
+        ([2.0], [np.int32(2)], 0),
+        (True, 1, 0),
+        ([1, 2], [3], 1),
+        (np.array([1]), 1, 1),
+    ],
+)
+def test_get_params_html_types(initial_value, instance_value, non_default_length):
+    class MyEstimator(BaseEstimator):
+        def __init__(self, initial_value=initial_value):
+            self.initial_value = initial_value
+
+    est = MyEstimator(initial_value=instance_value)
+    # when initial_value and instance_value are different, it means
+    # that instance_value is a non default value
+    assert len(est._get_params_html().non_default) == non_default_length
path: sklearn/tests/test_base.py
text: @user2:
Let's make this test a bit more generic to cover more cases:

```python
def make_estimator_with_param(default_value):
    class DynamicEstimator(BaseEstimator):
        def __init__(self, param=default_value):
            self.param = param

    return DynamicEstimator


@user1.mark.parametrize(
    "default_value, test_value",
    [
        ((), (1,)),
        ((), [1]),
        ((), np.array([1])),
        ((1, 2), (3, 4)),
        ((1, 2), [3, 4]),
        ((1, 2), np.array([3, 4])),
        (None, 1),
        (None, []),
        (None, lambda x: x),
        (np.nan, 1.0),
        (np.nan, np.array([np.nan])),
        ("abc", "def"),
        ("abc", ["abc"]),
        (True, False),
        (1, 2),
        (1, [1]),
        (1, np.array([1])),
        (1.0, 2.0),
        (1.0, [1.0]),
        (1.0, np.array([1.0])),
    ],
)
def test_param_is_non_default(default_value, test_value):
    """Check that we detect non-default parameters with various types.

    Non-regression test for:
    https://github.com/scikit-learn/scikit-learn/issues/31525
    """
    estimator = make_estimator_with_param(default_value)(param=test_value)
    non_default = estimator._get_params_html().non_default
    assert (
        "param" in non_default
    ), f"param should be non-default for {default_value!r} vs {test_value!r}"


@user1.mark.parametrize(
    "default_value, test_value",
    [
        (None, None),
        ((), ()),
        ((), []),
        ((), np.array([])),
        ((1, 2, 3), (1, 2, 3)),
        ((1, 2, 3), [1, 2, 3]),
        ((1, 2, 3), np.array([1, 2, 3])),
        (np.nan, np.nan),
        ("abc", "abc"),
        (True, True),
        (1, 1),
        (1.0, 1.0),
    ],
)
def test_param_is_default(default_value, test_value):
    """Check that we detect the default parameters and values in an array-like will
    be reported as default as well.

    Non-regression test for:
    https://github.com/scikit-learn/scikit-learn/issues/31525
    """
    estimator = make_estimator_with_param(default_value)(param=test_value)
    non_default = estimator._get_params_html().non_default
    assert (
        "param" not in non_default
    ), f"param should not be non-default for {default_value!r} vs {test_value!r}"
```

Those tests where indeed failing before in various manner when we had array and scalar or empty arrays.

@author:
1. Are the `f` strings after the end of each assertion needed? `, f"param should not be non-default for {default_value!r} vs {test_value!r}"` ? I see what it does, but it looks different from the rest of the code base. 

2. Jeremy had mentioned to use the snippet from the issue, you don't want that?  (lines 1007-1025) where I specifically use `RidgeCV(np.logspace(-3, 3, num=10))`

3. What do you mean here?: "Those tests where indeed failing before in various manner when we had array and scalar or empty arrays." .. when/where were they failing? 

Not sure if I have to ping you @user2 

@author:
Plus there are no tests with arrays of different sizes, which was the original issue/problem.

@user3:
> Jeremy had mentioned to use the snippet from the issue, you don't want that? (lines 1007-1025) where I specifically use RidgeCV(np.logspace(-3, 3, num=10))

That was a suggestion but using a generic estimator for all tests is more simple so why not

> Aren't some of the tests redundant? like 1.0 vs 2.0. Or comparing 1 vs 2, etc. I'm just double checking.

the goal is to test for different types (here int and float). Better have too man y tests than not enough :)

@user3:
> Plus there are no tests with arrays of different sizes, which was the original issue/problem

I think that it's worth adding the case of 2 arrays of different sizes

@user2:
> Are the f strings after the end of each assertion needed? , f"param should not be non-default for {default_value!r} vs {test_value!r}" ? I see what it does, but it looks different from the rest of the code base.

Basically, if the `AssertError` is shown, then it will print the error message. When it is trivial we never put anything but sometimes we are nice. But I agree that you can remove it here because what we assert it straightforward.

diff_hunk: @@ -0,0 +1,3 @@
+- Fix regression on the :class: `linear_model.RidgeCV` diagram representation
+  with non-default alphas.
path: doc/whats_new/upcoming_changes/sklearn.base/31528.fix.rst
text: @user1:
```suggestion
- Fix regression in HTML representation when detecting the non-default parameters
  that where of array-like types.
```