Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH add fista restart solver #121

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

HippolyteLBRRR
Copy link

No description provided.

solvers/fista_automatic_restart.py Outdated Show resolved Hide resolved
solvers/fista_automatic_restart.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@mathurinm mathurinm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Hippolyte, a few comments on variable names.
Do you have results on simple datasets to share ?

solvers/fista_automatic_restart.py Outdated Show resolved Hide resolved
solvers/fista_automatic_restart.py Outdated Show resolved Hide resolved
solvers/fista_automatic_restart.py Outdated Show resolved Hide resolved
solvers/fista_automatic_restart.py Outdated Show resolved Hide resolved
F_tab = np.zeros(int(n_iter/(2*C))+2)
i_glob = 0
i_int = 0
counter = 0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you try to have more explicit names like outer_iteration, inner_iteration etc?

solvers/fista_automatic_restart.py Outdated Show resolved Hide resolved
solvers/fista_automatic_restart.py Outdated Show resolved Hide resolved
@HippolyteLBRRR
Copy link
Author

Thank you for your comments I will correct that quickly!
I am still doing some tests but I will share my results when I have some!

Copy link
Member

@tomMoral tomMoral left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments but overall this is nice.

I am wondering if it would make sense to only compute the loss only after the restart, in the outer while loop? this would simplify the use of the callback.

solvers/fista_automatic_restart.py Outdated Show resolved Hide resolved
solvers/fista_automatic_restart.py Outdated Show resolved Hide resolved
solvers/fista_automatic_restart.py Outdated Show resolved Hide resolved
solvers/fista_automatic_restart.py Outdated Show resolved Hide resolved
solvers/fista_automatic_restart.py Outdated Show resolved Hide resolved
solvers/fista_automatic_restart.py Outdated Show resolved Hide resolved
solvers/fista_automatic_restart.py Outdated Show resolved Hide resolved
solvers/fista_automatic_restart.py Outdated Show resolved Hide resolved
solvers/fista_automatic_restart.py Outdated Show resolved Hide resolved
Copy link
Member

@tomMoral tomMoral left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good! :) A few more comments to make the code clearer.

Did you run a few benchmarks? Can you share one or 2 convergence curves (maybe with celer and modopt-fista['greedy']?

solvers/fista_automatic_restart.py Outdated Show resolved Hide resolved
solvers/fista_automatic_restart.py Outdated Show resolved Hide resolved
solvers/fista_automatic_restart.py Outdated Show resolved Hide resolved
solvers/fista_automatic_restart.py Outdated Show resolved Hide resolved
solvers/fista_automatic_restart.py Outdated Show resolved Hide resolved
solvers/fista_automatic_restart.py Outdated Show resolved Hide resolved
solvers/fista_automatic_restart.py Outdated Show resolved Hide resolved
@HippolyteLBRRR
Copy link
Author

"I am wondering if it would make sense to only compute the loss only after the restart, in the outer while loop? this would simplify the use of the callback."
I didn't see your remark sorry! I think that it is the way it is coded now but I may not understand your point.
I am still doing some tests but I will share some convergence curves as soon as possible!

@mathurinm
Copy link
Collaborator

I get a few division by zero errors, do you know where they come from @HippolyteLBRRR ?

/home/mathurin/workspace/benchs/lasso/solvers/fista_automatic_restart.py:66: RuntimeWarning: divide by zero encountered in divide                                                                
  mu = np.min(4 * L / (ite_per_restart[:n_restarts-1]+1) ** 2
/home/mathurin/workspace/benchs/lasso/solvers/fista_automatic_restart.py:66: RuntimeWarning: invalid value encountered in divide
  mu = np.min(4 * L / (ite_per_restart[:n_restarts-1]+1) ** 2

@HippolyteLBRRR
Copy link
Author

I get a few division by zero errors, do you know where they come from @HippolyteLBRRR ?

/home/mathurin/workspace/benchs/lasso/solvers/fista_automatic_restart.py:66: RuntimeWarning: divide by zero encountered in divide                                                                
  mu = np.min(4 * L / (ite_per_restart[:n_restarts-1]+1) ** 2
/home/mathurin/workspace/benchs/lasso/solvers/fista_automatic_restart.py:66: RuntimeWarning: invalid value encountered in divide
  mu = np.min(4 * L / (ite_per_restart[:n_restarts-1]+1) ** 2

I think that it comes from the fact that for a sufficient number of iterations, the value of the loss function does not vary much between two restarts. Consequently objectives[1:-1]-objectives[-1] (or F_tab[1:n_restarts] - last_F in the old version) may have some small coefficient which are interpreted as zeros. It does not have negative consequences on the algorithm as far as I know.

@HippolyteLBRRR
Copy link
Author

newplot
newplot
Here are two convergence curves for the dataset leukemia. Our restart strategy is not particularly efficient in that context compared to the other one and far behind Celer.

@tomMoral
Copy link
Member

Nice!! :)
It is cool that you are faster than Modopt greedy strategy.

Could you run the algorithm on larger datasets and with small reg (which tend to be more favorable to FISTA)?

@HippolyteLBRRR
Copy link
Author

newplot(1)
I have put two times the same graph, here it is for reg=0.05! Yes I will test the methods on other datasets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants