ENH - check `datafit + penalty` compatibility with solver #137

PABannier · 2022-12-10T18:03:11Z

A quick proof-of-concept of a function that checks if the combination (solver, datafit, penalty) is supported. Currently we have some edge cases where one can pass ProxNewton solver with L0_5 penalty without any error being raised.

Pros of this design: the validation rules are centralized and validating a 3-uple is a one-liner in glm_fit.
Cons: we have to update the rules as we enhance the capabilities of the solver.

All in all, I think it is very valuable to have more verbose errors when fitting estimators (e.g. Ali Rahimi initially passed a combination Quadratic, L2_3, ProxNewton which cannot be optimized at the moment of writing).

Closes #101
Closes #90
Closes #109

PABannier · 2022-12-10T18:07:20Z

With this PR, the errors are more verbose:

In [1]: from skglm.estimators import GeneralizedLinearEstimator
           from skglm.penalties import L0_5
           from skglm.datafits import Quadratic, Logistic
           from skglm.solvers import ProxNewton, AndersonCD
           import numpy as np

In [2]: X = np.random.normal(0, 1, (30, 50))
           y = np.random.normal(0, 1, (30,))

In [3]: clf = GeneralizedLinearEstimator(Quadratic(), L0_5(1.), ProxNewton())

In [4]: clf.fit(X, y)
---------------------------------------------------------------------------
Exception                                 Traceback (most recent call last)
Input In [4], in <cell line: 1>()
----> 1 clf.fit(X, y)

File ~/Documents/skglm/skglm/estimators.py:241, in GeneralizedLinearEstimator.fit(self, X, y)
    238 self.datafit = self.datafit if self.datafit else Quadratic()
    239 self.solver = self.solver if self.solver else AndersonCD()
--> 241 return _glm_fit(X, y, self, self.datafit, self.penalty, self.solver)

File ~/Documents/skglm/skglm/estimators.py:29, in _glm_fit(X, y, model, datafit, penalty, solver)
     27 is_classif = isinstance(datafit, (Logistic, QuadraticSVC))
     28 fit_intercept = solver.fit_intercept
---> 29 validate_solver(solver, datafit, penalty)
     31 if is_classif:
     32     check_classification_targets(y)

File ~/Documents/skglm/skglm/utils/dispatcher.py:21, in validate_solver(solver, datafit, penalty)
      6 """Ensure the solver is suited for the `datafit` + `penalty` problem.
      7
      8 Parameters
   (...)
     17     Penalty.
     18 """
     19 if (isinstance(solver, ProxNewton)
     20     and not set(("raw_grad", "raw_hessian")) <= set(dir(datafit))):
---> 21     raise Exception(
     22         f"ProwNewton cannot optimize {datafit.__class__.__name__}, since `raw_grad`"
     23         " and `raw_hessian` are not implemented.")
     24 if ("ws_strategy" in dir(solver) and solver.ws_strategy == "subdiff"
     25     and isinstance(penalty, (L0_5, L2_3))):
     26     raise Exception(
     27         "ws_strategy=`subdiff` is not available for Lp penalties (p < 1). "
     28         "Set ws_strategy to `fixpoint`.")

Exception: ProwNewton cannot optimize Quadratic, since `raw_grad` and `raw_hessian` are not implemented.

mathurinm · 2022-12-12T09:09:49Z

Looks nice @PABannier, this will definitely improve UX!

From an API point of view, shouldn't this check be delegated to each solver? This way we don't have one big function, but Solver.validate(datafit, penalty), in the spirit of what @Badr-MOUFAD implemented here : https://github.com/scikit-learn-contrib/skglm/blob/main/skglm/experimental/pdcd_ws.py#L201

Such functions could also take care of the initialization (e.g. stepsize computation) which is done on a solver basis. WDYT?

PABannier · 2023-01-07T12:02:32Z

@mathurinm Yes I think it's cleaner, currently refining the POC.

mathurinm · 2023-06-20T14:28:10Z

This would be a nice addition if we can ship it in the 0.3 release @Badr-MOUFAD , given that we added a few datafits, penalties and solvers !

mathurinm · 2023-10-06T13:32:41Z

@Badr-MOUFAD the issue popped up in #188, do you have time to take this over ? A simple check, at the beginning of each solver, that the datafit and penalty are supported (eg AndersonCD does not support Gamma datafit)

Badr-MOUFAD · 2023-10-18T08:34:42Z

@Badr-MOUFAD the issue popped up in #188, do you have time to take this over ? A simple check, at the beginning of each solver, that the datafit and penalty are supported (eg AndersonCD does not support Gamma datafit)

Sure, I will resume this PR.

…into solver_dispatcher

mathurinm · 2023-10-24T08:35:01Z

Requires #191 to be implemented fit to allow for better checks

QB3

What a massive piece of work! Congrats @PABannier @Badr-MOUFAD !

My complain would be that I did not fully understood the need of calling multiple functions in the _validate function in solvers/base.py.

skglm/penalties/non_separable.py

QB3 · 2023-11-23T16:55:59Z

skglm/solvers/base.py

+        X : array, shape (n_samples, n_features)
+            Training data.
+
+        y : array, shape (n_samples,)


custom_compatibility_check currently depends on the target y, but the target is never used in the check.
Should we remove y from this function, or do you see cases where it will be needed?

Since the validation of datafit/penalty depends on the data, for instance when X is sparse we should check that the datafit implements _sparse methods, IMO it is better to pass in both X, y

For now, we can settle for X only, but that means adding y later if we need it which would alter the API.

I agree with @Badr-MOUFAD here, even if not needed at the moment it's not too hard to see cases where this would happen, and an API change will be painful

QB3 · 2023-11-23T17:10:02Z

skglm/solvers/base.py

+
+    def _validate(self, X, y, datafit, penalty):
+        # execute both: attributes checks and `custom_compatibility_check`
+        self.custom_compatibility_check(X, y, datafit, penalty)


From a conceptual point of view I am a bit confused.

The doc says custom_compatibility_check already checks the compatibility between the solver, the dataset and the penalty. Thus I do not understand the need to call multiple functions check_obj_solver_attr and check_obj_solver_attr? Does one check generic compatibility and the other check custom compatibility?

Does one check generic compatibility and the other check custom compatibility?

Yes indeed. Generic checks are checks for datafit/penalty implement the required attributes in _datafit_required_attr/_penalty_required_attr.
On the other hand, custom_compatibility_check does other checks, for instance in GramCD it checks that datafit is instance of Quadratic

one of confusions for me is from the name, check_obj_solver_attr could be just check_attr; we pass solver just to get its name, though useful I don't think it is worth it.

skglm/solvers/fista.py

skglm/experimental/pdcd_ws.py

skglm/solvers/group_bcd.py

skglm/solvers/group_prox_newton.py

Co-authored-by: Quentin Bertrand <quentin.bertrand@mila.quebec>

…ver_dispatcher

mathurinm · 2024-04-11T09:15:40Z

skglm/solvers/base.py

+
+    Attributes
+    ----------
+    _datafit_required_attr : list


I think we can make these public @BadrMOUFAD ? Maybe I'm missing a specific reason

also typo missing "that must BE" here and below

I think we can make these public @BadrMOUFAD ? Maybe I'm missing a specific reason

I think these two attributes should be read only.

While there is a way to make attributes read only, namely using the property decorator, I believe it adds two much complexity to the code and hence doesn’t serve our goal to make components implementation user-friendly.

I opted for the “start with underscore” naming convention to make variables private to signal to the user that these are attributes to not mess up with.

mathurinm · 2024-04-11T09:16:10Z

skglm/solvers/base.py

+        X : array, shape (n_samples, n_features)
+            Training data.
+
+        y : array, shape (n_samples,)


I agree with @Badr-MOUFAD here, even if not needed at the moment it's not too hard to see cases where this would happen, and an API change will be painful

mathurinm · 2024-04-11T09:25:00Z

skglm/utils/validation.py

+    missing_attrs = []
+    suffix = SPARSE_SUFFIX if support_sparse else ""
+
+    # if `attr` is a list check that at least one of them


why "at least one of them" ?

mathurinm · 2024-04-11T09:34:17Z

skglm/experimental/pdcd_ws.py

-        if issparse(X):
-            raise ValueError("Sparse matrices are not yet support in PDCD_WS solver.")
+        # jit compile classes
+        datafit = compiled_clone(datafit_)


I think this is done only for this solver, is there a particular reason?

Yes you are right.

I removed it.
One thing is that is might have some side effects (for user) as the compilation was done in _validate_init.

mathurinm · 2024-04-11T09:38:20Z

skglm/solvers/base.py

+
+    Notes
+    -----
+    For required attributes, if an attribute is given as a list of attributes


I had trouble understanding this, an example help (in which case do we want to check that one of several attributes is present?)

I 100% agree with you @mathurinm, I should have accompanied the docs with an example

In Fista solver, this

_datafit_required_attr = ("get_global_lipschitz", ("gradient", "gradient_scalar")) _penalty_required_attr = (("prox_1d", "prox_vec"),)

is interpreted as

datafit is required to have get_global_lipschitz and (gradient or gradient_scalar)

penalty is required to have prox_1d or prox_vec

This is the way I implemented check_obj_solver_attr function: whenever attributes are wrapped in parenthesis, it is interpreted as the “or” operator and a comma is interpreted the “and” operator.

mathurinm · 2024-04-11T09:41:41Z

skglm/solvers/fista.py

@@ -27,6 +28,9 @@ class FISTA(BaseSolver):
           https://epubs.siam.org/doi/10.1137/080716542
    """

+    _datafit_required_attr = ("get_global_lipschitz", ("gradient", "gradient_scalar"))
+    _penalty_required_attr = (("prox_1d", "prox_vec"),)


why does FISTA need prox_1D, it is not used in the code below

It is used in _prox_vec which take as argument the penalty (cf. https://github.com/PABannier/skglm/blob/e687bc2ecaacfe920b9aaad3e33e1f0cbdbac683/skglm/solvers/fista.py#L83)

The algorithm works if penalty has either of prox_1d or prox_vec.
(for reference #137 (comment))

mathurinm · 2024-04-11T09:42:03Z

skglm/solvers/gram_cd.py

+    def custom_compatibility_check(self, X, y, datafit):
+        if not isinstance(datafit, Quadratic):
+            raise AttributeError(
+                f"`GramCD` supports only `Quadratic` datafit, got {datafit}"


this is very clean

skglm/solvers/multitask_bcd.py

skglm/solvers/prox_newton.py

skglm/utils/validation.py

mathurinm · 2024-04-11T09:43:57Z

Trying to revive this to release v0.4. Some comments for discussion @QB3 @Badr-MOUFAD

I feel strongly against having both solver.solve() and solver(). "There should be one-- and preferably only one --obvious way to do it".
The way I see it, all solve methods should be renamed _solve, and BaseSolver should implement its solve that does the checks then call _solve. What do you think? This is non-breaking from the API point of view, just requires people implementing their own solver to adapt (I have no such example in mind ; we can implement a test that no Solver implements solve by checking that A.solve == BaseSolver.solve)
BaseSolver.solve can have a run_checks method which is True by default, and disabled if one wants to be faster (any idea on the impact of the checks @Badr-MOUFAD ?)
I feel like checking for the sparse support is heavy at the moment and could be done in another PR, but that may just be me :)
I have mixed feelings for the name check_obj_solver_attr; see my comment, I'd go for check_attr and not pass solver, which is used only for its name.
custom_compatibility_check could be custom_checks (?)

Badr-MOUFAD · 2024-05-24T11:22:32Z

I’m +1 with having _solve method and adding run_checks argument to solve method.
I feel it give us more freedom to standardize the behavior and make less verbose the solve method.
I don’t think the check have a big overhead, though I didn’t check that in practice
The support of sparse data is covered in check_obj_solver_attr function, so I don’t think it hurts us to cover it in this PR
I have no strong opion about the names, I’m +1 with your proposed name @mathurinm

initial commit

0154215

PABannier changed the title ~~POC Add validation logic passing datafit, penalty and solver~~ POC Add validation logic when passing datafit, penalty and solver to _glm_fit Dec 10, 2022

PABannier marked this pull request as draft December 11, 2022 19:02

PABannier added 2 commits January 7, 2023 16:43

delegated to solve

3c70b85

call solver.validate

ee0d29c

mathurinm mentioned this pull request Oct 6, 2023

ENH - implement intercept update for Poisson and Gamma datafits #189

Merged

Badr-MOUFAD closed this in #189 Oct 18, 2023

mathurinm reopened this Oct 18, 2023

Badr-MOUFAD added 11 commits October 18, 2023 10:48

Merge branch 'main' of https://github.com/scikit-learn-contrib/skglm …

8831cd2

…into solver_dispatcher

add validate method to solvers

9abd149

implem validation logic

d20229a

implem attribute validation for solvers

310c572

validation PDCD_WS

1872e4e

fix trailing spaces

cd1ba1c

add docs to check_obj_solver_compatibility

4c9b4d4

add validation glm_fit

c3d01c4

fix Error logs

eacea14

fix prox solvers attribute names

573cb78

add initialize to required attributes

8dedf18

Badr-MOUFAD changed the title ~~POC Add validation logic when passing datafit, penalty and solver to _glm_fit~~ ENH - check datafit + penalty compatibility with solver Oct 18, 2023

Badr-MOUFAD added 2 commits October 18, 2023 15:18

add change to what's new

19fbe63

formatting & Fista validation

3b65445

Badr-MOUFAD requested a review from mathurinm October 18, 2023 13:47

Badr-MOUFAD added 4 commits November 16, 2023 16:52

more on unittest

bab6277

fix what's new

7b79539

use __call__ instead of solve

118a0da

fix BaseSolver

94f47c2

Badr-MOUFAD added Ready for review and removed Work In Progress labels Nov 16, 2023

QB3 reviewed Nov 23, 2023

View reviewed changes

Badr-MOUFAD and others added 5 commits November 24, 2023 09:59

Update skglm/solvers/group_bcd.py

419df06

Co-authored-by: Quentin Bertrand <quentin.bertrand@mila.quebec>

Update skglm/solvers/group_prox_newton.py

a26387b

Co-authored-by: Quentin Bertrand <quentin.bertrand@mila.quebec>

Update skglm/experimental/pdcd_ws.py

677d0e3

Co-authored-by: Quentin Bertrand <quentin.bertrand@mila.quebec>

Merge branch 'main' of github.com:scikit-learn-contrib/skglm into sol…

5fb3994

…ver_dispatcher

chenges.rst

8f840bf

mathurinm reviewed Apr 11, 2024

View reviewed changes

skglm/solvers/multitask_bcd.py Outdated Show resolved Hide resolved

mathurinm reviewed Apr 11, 2024

View reviewed changes

skglm/solvers/prox_newton.py Outdated Show resolved Hide resolved

mathurinm reviewed Apr 11, 2024

View reviewed changes

skglm/utils/validation.py Outdated Show resolved Hide resolved

mathurinm and others added 8 commits April 11, 2024 13:02

sparse matrices are now supported by GroupBCD

d8cc022

Merge branch 'main' into solver_dispatcher

8e41a5d

typo BaseSolver

fa9ed35

rm self in docs

e687bc2

more code-readable attribute error

fd67df8

rm data compilation in PDCD_WS

78295d8

rm unused imports

2003370

fix test PDCD_WS

a2aa8f5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH - check `datafit + penalty` compatibility with solver #137

ENH - check `datafit + penalty` compatibility with solver #137

PABannier commented Dec 10, 2022 •

edited

PABannier commented Dec 10, 2022 •

edited

mathurinm commented Dec 12, 2022

PABannier commented Jan 7, 2023

mathurinm commented Jun 20, 2023

mathurinm commented Oct 6, 2023

Badr-MOUFAD commented Oct 18, 2023

mathurinm commented Oct 24, 2023

QB3 left a comment

QB3 Nov 23, 2023 •

edited

Badr-MOUFAD Nov 24, 2023

mathurinm Apr 11, 2024

QB3 Nov 23, 2023 •

edited by Badr-MOUFAD

Badr-MOUFAD Nov 24, 2023

mathurinm Apr 11, 2024

mathurinm Apr 11, 2024

mathurinm Apr 11, 2024

Badr-MOUFAD May 24, 2024

mathurinm Apr 11, 2024

mathurinm Apr 11, 2024

mathurinm Apr 11, 2024

Badr-MOUFAD May 24, 2024

mathurinm Apr 11, 2024

Badr-MOUFAD May 24, 2024

mathurinm Apr 11, 2024

Badr-MOUFAD May 24, 2024

mathurinm Apr 11, 2024

mathurinm commented Apr 11, 2024

Badr-MOUFAD commented May 24, 2024

ENH - check datafit + penalty compatibility with solver #137

Are you sure you want to change the base?

ENH - check datafit + penalty compatibility with solver #137

Conversation

PABannier commented Dec 10, 2022 • edited

PABannier commented Dec 10, 2022 • edited

mathurinm commented Dec 12, 2022

PABannier commented Jan 7, 2023

mathurinm commented Jun 20, 2023

mathurinm commented Oct 6, 2023

Badr-MOUFAD commented Oct 18, 2023

mathurinm commented Oct 24, 2023

QB3 left a comment

Choose a reason for hiding this comment

QB3 Nov 23, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QB3 Nov 23, 2023 • edited by Badr-MOUFAD

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathurinm commented Apr 11, 2024

Badr-MOUFAD commented May 24, 2024

ENH - check `datafit + penalty` compatibility with solver #137

ENH - check `datafit + penalty` compatibility with solver #137

PABannier commented Dec 10, 2022 •

edited

PABannier commented Dec 10, 2022 •

edited

QB3 Nov 23, 2023 •

edited

QB3 Nov 23, 2023 •

edited by Badr-MOUFAD