FEAT - Sample weights

[sujay-pandit]

Description of the feature

Sklearn 'fit' functions have an option to pass 'sample_weights' array to assign importance to samples and modify the cost function accordingly.

Additional context
Wanted to use 'https://github.com/SteiMi/denseweight' with MCP.

[mathurinm]

Hi @sujay-pandit, thanks for reaching out.

At first sight it should be doable to add a sample_weights attribute to a datafit, say Quadratic, and to take it into account:

• when computing the datafit value
• when computing the datafit gradient
• when computing the lipschitz constant

The calculation should simply amount to appropriate mutliplications by sample_weights[i]

Would you like to send a Pull request to start working on it? We can guide you and help you through the process

[sujay-pandit]

Yes, I can take that up. Thanks!
Can you start a branch for me?

[mathurinm]

If I start the branch, you won't be able to push to it. You need to fork the repository, and push to a branch in your fork, and then launch a pull request here.
see https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-a-pull-request-from-a-fork

ChatGPT may also help on that matter :)

[mathurinm]

@sujay-pandit scikit-learn has a great explanation for pull requests, you can follow it (adapting the URLs, eg replacing scikit-learn by skglm and others) : https://scikit-learn.org/stable/developers/contributing.html#how-to-contribute

[sujay-pandit]

Got it! Thanks @mathurinm

[kmedved]

Any update here? This is a key feature for many problems, so am likewise very interested.

[mathurinm]

Pinging @sujay-pandit on this topic

[sujay-pandit]

Hey @kmedved and @mathurinm,
I did get started on it but need to finish some work for my paper submission in April. I will start working on this as soon as possible.

[mathurinm]

Thanks @sujay-pandit
If you already started writing some code, can you please open a draft PR, so that other contributors can give early feedback and build on this work?

[sujay-pandit]

Hi @mathurinm and @kmedved
Apologies for the delay in communication. I am starting afresh now.
Before I make changes, I wanted to get a confirmation of my plan. For now, my focus is on implementing this for MCPRegression, which I can later extend to others.
I think majority of my changes are going to be in the datafit (Quadratic) class while computing loss and gradient w.r.t to weights. Let me know if the plan below looks ok.

1. I will add a parameter to MCPRegression constructor called 'sample_weights', which will default to None. [class MCPRegression, estimators.py]

2. The same parameter will also be added to the solver [AndersonCD, anderson_cd.py]

3. I feel the majority of my changes are here, in datafit class [class Quadratic, single_task.py]

   Datafit (single_task.py)
   Below are the changes I have in mind. I will scale the loss computation by sample_weights in the value method.
   And, while computing gradients per weight (w_j) in the gradient_scalar method as follows:

   class Quadratic(BaseDatafit):
       def __init__(self, sample_weights=None):    
           self.sample_weights = sample_weights  
   
       def value(self, y, w, Xw):  
           if self.sample_weights is not None:  
               return np.sum((self.sample_weights * (y - Xw) ** 2)) / (2 * len(y))  
           else:  
               return np.sum((y - Xw) ** 2) / (2 * len(y))  
   
       def gradient_scalar(self, X, y, w, Xw, j):  
           if self.sample_weights is not None:  
               return (X[:, j] * (Xw - y) * self.sample_weights).sum() / len(y)  
           else:  
               return (X[:, j] * (Xw - y)).sum() / len(y)

Question: I am not expecting my penalty(MCPenalty/WeightedMCPenalty) classes to change. Is my understanding correct?

Thanks!

[QB3]

Hello @sujay-pandit ,
Thanks a lot for the incoming PR. My plan would be:

• 1 First, add the sample_weights to the Quadratic class, note that you will also have to change the Lipschitz constant computation (some work has already been started in Implement sample weights in Quadratic class #245 )
• 2 Then, before touching the estimator, I would check that the created datafit works a penalty (L1, MCP) with AndersonCD, like done in this example https://contrib.scikit-learn.org/skglm/auto_examples/plot_sparse_recovery.html#sphx-glr-auto-examples-plot-sparse-recovery-py
• 3 Finally, modify the MCPRegression constructor as you said

IMO sample_weights should not be passed to AndersonCD: sample_weights should be passed to Quadratid, which itself will be passed to the solver.

Question: I am not expecting my penalty(MCPenalty/WeightedMCPenalty) classes to change. Is my understanding correct?

Yes indeed! No penalty should be modified when adding this new feature.

Quick question from my side: for the design, do we want to modify the Quadratic class, or to create a WeightedQuadratic class, which would inherit from Quadratic ? (I would vote for a new class)

[sujay-pandit]

Hi @QB3
Thanks for the prompt feedback!

• 1 First, add the sample_weights to the Quadratic class, note that you will also have to change the Lipschitz constant computation (some work has already been started in Altered Quadratic Class #245 )

Yes. you are right; for Lipschitz computation, I will have to scale the computed values by sample weights.

• 2 Then, before touching the estimator, I would check that the created datafit works a penalty (L1, MCP) with AndersonCD, like done in this example https://contrib.scikit-learn.org/skglm/auto_examples/plot_sparse_recovery.html#sphx-glr-auto-examples-plot-sparse-recovery-py

Sounds good

• 3 Finally, modify the MCPRegression constructor as you said

IMO sample_weights should not be passed to AndersonCD: sample_weights should be passed to Quadratid, which itself will be passed to the solver.

Agreed, that will be cleaner.

Quick question from my side: for the design, do we want to modify the Quadratic class, or to create a WeightedQuadratic class, which would inherit from Quadratic ? (I would vote for a new class)

Are there any more changes planned along with sample weights? I don't think the addition of sample weights is going to need a lot of changes to the existing class. If not, I would vote to just modify the existing class.

[mathurinm]

My take: To support both classic Quadratic and weighted Quadratic in one class I see only two options:

• have a Quadratic.weights attribute, which is None if no weights are used and a np.array of shape (n_samples,) otherwise. We keep the existing behavior if self.weights is None. This is implemented in Implement sample weights in Quadratic class #245, but unfortunately I don't think this mixed typing (None or np.array) is supported by numba, see the test failure here : https://github.com/scikit-learn-contrib/skglm/actions/runs/8631223494/job/23660827782?pr=245#step:6:375
• have a Quadratic.weights attribute, which is np.ones(n_samples) if there are no weights, and a np.array of shape (n_samples,) otherwise. I'm afraid this would slow down the code in the unweighted case; I also don't see how to set the weights to 1 by default (without using a mutable default parameter value, which is dangerous)

So it's probably easier to go for two separate classes, Quadratic and WeightedQuadratic. This should make the PR straightforward.

[sujay-pandit]

@mathurinm and @QB3,
In this case, we can go for separate classes. I wasn't aware of this numba issue.
I have a question,
The changes in #245 and this issue will clash. Can we keep just one of them? Or should I wait until the changes in #245 are complete?

[mathurinm]

#245 is stalled, you can start a separate PR.

You can even start from it, to reuse existing work.
To do so first fetch the PR branch, then switch to a new branch:

git fetch git@github.com:scikit-learn-contrib/skglm pull/245/head:nbb
git switch nbb
git switch -c your_desired_branch_name

and start working as usual on a branch.