Fix complex support for L-BFGS - #1142
Conversation
|
Hey @gautierronan, |
|
@vroulet Should be good for review. Note that, in the test, I have commented one linesearch option because the test was not passing, but I don't think it's related to complex support. |
vroulet
left a comment
There was a problem hiding this comment.
That looks great, thank you @gautierronan ! I left you some cosmetic comments.
In addition, can you add the following note in the docstrings of lbfgs, scale_by_zoom_linesearch, scale_by_backtracking_linesearch (please compile the docs to verify that it's well formatted)?
..note:: The algorithm can support complex inputs.
Ideally, one would add a simple example in the lbfgs notebook to link this note to an implementation (as you pointed out the gradients need to pass through a conjugate). Let me know if you could add that too.
Also could you add the test on the complex rosenbrock that was in jaxopt. It should amount to add exactly
def test_lbfgs_complex_rosenbrock(self):
# Taken from previous jax tests
tol = 1e-5
complex_dim = 5
fun_real = _get_problem('rosenbrock')['fun']
init_real = jnp.zeros((2 * complex_dim,), dtype=complex)
expected_real = jnp.ones((2 * complex_dim,), dtype=complex)
def fun(z):
x_real = jnp.concatenate([jnp.real(z), jnp.imag(z)])
return fun_real(x_real)
init = init_real[:complex_dim] + 1.j * init_real[complex_dim:]
expected = expected_real[:complex_dim] + 1.j * expected_real[complex_dim:]
opt = alias.lbfgs()
got, _ = _run_opt(opt, fun, init, maxiter=500, tol=tol)
chex.assert_trees_all_close(got, expected)
If it does not pass, let me know. I can work on that in a separate PR once yours is merged.
f19e726 to
db4ff3e
Compare
|
Thanks for the review. All comments should be addressed in the last commits. Let me know if that works.
I don't think I will have time for this. Also, it's IMO expected that gradients need to pass through a conjugate (nothing specific to L-BFGS), and is already the behavior in the other optax functions I believe. So a tutorial dedicated to complex-valued gradient descent might be more relevant than a specific addition to the L-BFGS tutorial? |
Good point. We can make an issue for that. Thanks for all! |
Closes #1141.
Not 100% sure that this doesn't break other things or fully works, but at least the MWE below seems to work fine.
Notice the
solve.updatecall which requires ajnp.conj(grad)twice. I believe this is correct and aligned with other optax solvers, but not sure either.