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.

Sign up for GitHub

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

Jump to bottom

tadam solver #260

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

tadam solver #260

wants to merge 5 commits into from

Conversation

d-monnet
Copy link
Contributor

@d-monnet d-monnet commented Mar 6, 2024

Trust-region embedded ADAM (TADAM) is an adaptation of ADAM which converges in the non-convex case.
It relies on limiting the momentum contribution to ensure that the step is along a descent direction.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Mar 6, 2024 •
edited
Loading

Package name latest stable
FletcherPenaltySolver.jl
Percival.jl

@d-monnet
Copy link
Contributor Author

d-monnet commented Mar 6, 2024 •
edited
Loading

There are surely allocation issues due to solve_tadam_subproblem that will have to be fixed.
Some tests do not pass with Float16, but the method is not well-suited for deterministic problems.

@d-monnet d-monnet requested review from tmigot and dpo March 8, 2024 18:13
@tmigot
Copy link
Member

tmigot commented Mar 9, 2024

Did you investigate why the tests didn't pass?

dpo
dpo reviewed Mar 9, 2024
View reviewed changes
src/tadam.jl
Trust-region embeded ADAM (TADAM) algorithm for unconstrained optimization. This is an adaptation of ADAM which enforces convergence in the non-convexe case.

# Minimal algorithm description
The step sk at iteration k is computed as:
Copy link
Member

Choose a reason for hiding this comment

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

Please apply all the same kinds of comments as for FOMO related to spacing, blank lines, indentation, etc.

src/tadam.jl Outdated Show resolved Hide resolved
src/tadam.jl Show resolved Hide resolved
src/tadam.jl
∇fk = solver.∇f
c = solver.c
momentum = solver.m
d̂ = solver.d
Copy link
Member

Choose a reason for hiding this comment

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

Could it just be called d? The hat is displayed in a strange way.

@d-monnet
Copy link
Contributor Author

Did you investigate why the tests didn't pass?

tadam is an adaptation of ADAM, and ADAM is meant to train DNN in a stochastic context and tends to generate steps $s = -sign(\nabla f(x))$.
It looks like tadam is not very well adapted to solve deterministic problems and might even fail to solve simple ones.
I haven't looked into the detail why tadam fails for the low precision test (Float16), but it's very probably related to the above.

@tmigot
Copy link
Member

tmigot commented Mar 11, 2024

I understand. If it's just the Float16 bugging, then most likely it is a tolerance or parameter issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpo dpo dpo left review comments

@tmigot tmigot Awaiting requested review from tmigot

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@d-monnet @tmigot @dpo