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

BestSoFarMinimiser behavior #33

Open
ColCarroll opened this issue Jan 3, 2024 · 1 comment · May be fixed by #34
Open

BestSoFarMinimiser behavior #33

ColCarroll opened this issue Jan 3, 2024 · 1 comment · May be fixed by #34
Labels
refactor Tidy things up

Comments

@ColCarroll
Copy link
Contributor

Not sure if this is a bug or not, but BestSoFarMinimiser appears to not check the last step of the wrapped solver:

solver = optimistix.BestSoFarMinimiser(optimistix.BFGS(rtol=1e-5, atol=1e-5))
ret = optimistix.minimise(lambda x, _: (x - 3.)**2, solver, 0.)
print(ret.value, ret.state.state.y_eval)

0.0 3.0
@patrick-kidger
Copy link
Owner

Bother. This is actually doing what the BestSoFar* wrappers aim to do, but I can see that it's not really optimal. To be precise: during optimisation, we get a sequence of values y0, y1, y2, ..., y_{n-1}, y_n. Typically we evaluate f(y_i) and from its value determine how to choose y_{i+1}. The goal of the BestSoFar* wrappers is to return the y_i for which f(y_i) is the smallest value we ever see. However, we never actually evaluate f(y_n), the very last iterate! So the wrappers never look at that value.

I think the fix should be to perform that evaulation in postprocess:

def postprocess(
self,
fn: Fn[Y, Out, Aux],
y: Y,
aux: Aux,
args: PyTree,
options: dict[str, Any],
state: _BestSoFarState,
tags: frozenset[object],
result: RESULTS,
) -> tuple[Y, Aux, dict[str, Any]]:
return state.best_y, state.best_aux, {}

so that its body looks something like:

f, aux = fn(y, args)
loss = self._to_loss(y, f)
pred = loss < state.best_loss
best_y = tree_where(pred, y, state.best_y)
best_aux = tree_where(pred, new_aux, state.best_aux)
return best_y, best_aux, {}

What do you think? If that looks reasonable then I'd be happy to take a PR on this.

@patrick-kidger patrick-kidger added the refactor Tidy things up label Jan 5, 2024
@ColCarroll ColCarroll linked a pull request Jan 6, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Tidy things up
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants