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

pre_update for OptaxSolver run function? #27

Closed
FerranAlet opened this issue Sep 10, 2021 · 6 comments · Fixed by #29
Closed

pre_update for OptaxSolver run function? #27

FerranAlet opened this issue Sep 10, 2021 · 6 comments · Fixed by #29

Comments

@FerranAlet
Copy link

FerranAlet commented Sep 10, 2021

Hi,
Defining a pre_update function works well in OptaxSolver when using the run_iterator function (like in the MNIST example). However, it does not work with the run function. I checked the implementation and indeed there is no call to the pre_update inside 'run' but there is one inside 'run_iterator'. Is this a small bug or is this by design?

Thanks!

@mblondel
Copy link
Collaborator

mblondel commented Sep 13, 2021

Indeed, pre_update is currently only used in run_iterator and not in run. I was a bit hesitant about this, I am not sure what are the right semantics here. If we add it to run, then all solvers, including non-stochastic ones, will need a pre_update option. Is it worth it for batch solvers?

Another option would be to apply pre_update inside update. This means that every solver with a pre_update option would be in charge of calling it. This way, both run and run_iterator would use pre_update as a result. And this way pre_update doesn't 'leak' everywhere in the code base.

So the question is: do we make pre_update an integral part of our solver API or not?

Any opinion on this would be welcome.

@FerranAlet
Copy link
Author

I may be misinterpreting the role of pre_update (in particular, why it is significantly more useful in stochastic solvers).
My main use of the pre_update was tracking the progress of some running stats as well as how far along it is within the optimization (to know how much longer I have to wait). If pre_update is the only way to do this, I think it should be added everywhere, since tracking stats&progress seems pretty useful every time we have a solver. If we don't add it then I would put some sort of warning (or change the name?) in the OptaxSolver definition, because intuitively one would expect the pre_update to run if you give it to the OptaxSolver, whenever you call update.

Happy to chat more about it!

@mblondel
Copy link
Collaborator

Moving the call to pre_update to within OptaxSolver.update sounds good. Do you want to do it?

@mblondel
Copy link
Collaborator

I'm doing some changes in the base classes, I will tackle this issue too.

@mblondel
Copy link
Collaborator

Done in #29, opinions welcome!

@FerranAlet
Copy link
Author

Awesome! Thanks for solving it :)

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 a pull request may close this issue.

2 participants