Implement Ridge .solver_ estimated attribute

[csadorf]

Adds the .solver_ estimated attribute in addition to the .solver hyperparameter.

Switches the default cuml solver hyperparameter from "eig" to "auto" (backwards-compatible).

[copy-pr-bot]

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

[csadorf]

/ok to test

[csadorf]

/ok to test

[csadorf]

/ok to test

[csadorf]

@dantegd @betatim There are some potentially unrelated test failures, but this is ready for initial review.

[jcrist]

2 general comments:

• Presumably this is fixing a bug/increasing compatibility within cuml.accel. Can you add a test for this behavior?
• From the comments in this PR alone I cannot tell what issue this PR resolves or why it's being made. In the future can you try and include comments about why a PR is being made, why the approach you took solves the issue, etc...? This helps immensely with reviewing PRs and disseminating knowledge about why things are the way they are across the team. It also helps when future us run git blame to find out why this code was written :)

[csadorf]

I just implemented a few additional basic tests and it turns out that the cd solver is apparently not actually implemented. This was also noted in #6066 . I think we should remove that option for 25.04.

The cd solver maps to algo=2

cuml/python/cuml/cuml/linear_model/ridge.pyx

Line 254 in 3a8ea8c

'cd': 2

which then triggers this error:

cuml/cpp/src/glm/ridge.cuh

Lines 214 to 215 in 3a8ea8c

	} else if (algo == 2) {
	ASSERT(false, "ridgeFit: no algorithm with this id has been implemented");

@dantegd Do you have more context for this?

[betatim]

The fact that the attribute isn't equal to the value the user passed in is a curiosity of the current proxying implementation. How can we note that in the test so that when we change the implementation we know to modify this test/delete it? Leave a comment for people from the future?

[csadorf]

It is and it isn't. The skl implementation will also have a deviation in case that the default "auto" value is selected.

I further do not think that this is an artifact of our proxy implementation, but an artifact of the fundamental function of the acceleration mode. We can decide that the acceleration mode must faithfully obey all parameters 100%, including the solver selection in which case we would have to fall-back to CPU mode in case that a solver is selected that is not supported on the GPU. However, if we deem that selecting an equivalent suitable solver is acceptable then I don't think we should pretend that a specific skl-solver was used, when – in fact – it wasn't.

To be clear, I am not arguing that this is the right approach, but I am arguing that the deviation is not just an artifact of the current proxy layer implementation. I don't think that we need any further comment to remind us here, because this test will fail if we make an engineering decision to change the behavior.

[betatim]

The solver attribute will be set to "auto" in scikit-learn. The solver_ attribute will contain the name of the solver that was actually used.

In [20]: from sklearn.linear_model import Ridge

In [21]: r = Ridge()

In [22]: from sklearn.datasets import make_classification, make_regression

In [23]: X, y = make_regression()

In [24]: r.fit(X, y)
Out[24]: Ridge()

In [25]: r.solver
Out[25]: 'auto'

In [26]: r.solver_
Out[26]: 'cholesky'

I don't think we need to select the same solver, but I do think we should work towards passing the scikit-learn common tests which require that all constructor arguments are stored in attributes and that those remain unchanged.

[csadorf]

I don't understand your point, that is the exact behavior I was describing:

The skl implementation will also have a deviation in case that the default "auto" value is selected.

[betatim]

I somehow missed the first line/mixed it with the second of the test parametrisation :-/ so I thought that you pass "auto" but then the attribute is set to "eig".

[csadorf]

/merge