-
Notifications
You must be signed in to change notification settings - Fork 112
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
[line-search] implement Armijo line search algorithm #925
Conversation
Codecov Report
|
Hi Hendrick, this already looks very good. Could you add a reference for the Armijo-Goldstein line search? Also, I would be very happy if you could change the names of the arguments in the armijo rule to a more general setting. Please have a look at the requested changes. Tim |
Hi Tim, thank you for your suggestions! I will have a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @HenKlei, thanks a lot for this PR. This is looking very good already .. - You should add an entry to AUTHORS.md
..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from minor issues, I think this can be merged soon. @HenKlei, you should still add an entry to AUTHORS.md
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is all looking very nice now. Only one thing remains: there should be some form of test for the new code. There is already something very basic in pymortests.algorithms.stuff
. Could you add something there?
I have an idea for a test, I will work on that. |
I added a small test in de2747b, but actually, I am a bit astonished about the behavior of the Newton algorithm in this case. Using 0.5 as initial value does not work without using the line search method. Certainly, as far as I can see, the algorithm should converge even when using 0.5 as initial guess. Maybe, there's something wrong with the monomials, or I didn't get the idea of the tests in |
@HenKlei, turns out the implementation of def jacobian(self, U, mu=None):
return NumpyMatrixOperator(self.derivative(U.to_numpy()).reshape((1,1))) Can you fix it in this PR? |
Alright, I'm going to fix this and have another look at the tests. Thanks for the clue. |
@sdrave After applying the check you suggested, the test is not passing. This makes completely sense when looking at the problem: for instance the monom of degree 2, i.e. x^2, reaches the desired absolute tolerance of 1e-15 already for values like x=1e-8. Hence, comparing this value of x to 0 will lead to a failing test, although the correct root is approximated. I think, this test is maybe not the best for the Newton algorithm at all, at least in the way it is currently performed. Do you have any suggestions on how to proceed? |
Not at all ...
Maybe the easiest way would be to check whether the solution actually has a residual smaller than |
@HenKlei, I see you have used |
Now, the test should work as expected. @sdrave Maybe you could have another look. From my point of view, this can be merged. |
The |
I had another look at def test_lincomb_op():
p1 = MonomOperator(1)
p2 = MonomOperator(2)
p12 = p1 + p2
p0 = p1 - p1
x = np.linspace(-1., 1., num=3)
vx = p1.source.make_array((x[:, np.newaxis]))
assert np.allclose(p0.apply(vx).to_numpy(), [0.])
assert np.allclose(p12.apply(vx).to_numpy(), (x * x + x)[:, np.newaxis])
assert np.allclose((p1 * 2.).apply(vx).to_numpy(), (x * 2.)[:, np.newaxis])
assert almost_equal(p2.jacobian(vx).apply(vx), p1.apply(vx) * 2.).all() To me, the last line does not really make sense. I think, Nevertheless, I did not get the test to work again so far. The dimensions of the vectors and operators do not seem to fit. @sdrave Are you sure that the reshape in |
|
Alright, then I should be able to fix that. Thank you! |
@sdrave The tests pass. Just two small questions: |
@HenKlei, thanks for all the additional work!
Probably. Although I would do it in a separate PR.
I think this is not our nicest test, but for now I would leave it as is. @renefritze, is currently changing a lot in the test suite and I would wait a bit until things have settled down. I would say, let's merge this PR. |
Alright, sounds good! |
When solving nonlinear problems for several different parameters, finding a proper value for the
relax
parameter of the Newton solver that works for every parameter is quite difficult. Thus, I implemented a (very) simple line search algorithm that, at least in my application, led to convergence of the Newton method for every parameter.To check the original Armijo-Goldstein condition, one needs the gradient of the objective function. Thus, if no gradient is provided, a weaker version of this condition is checked; namely that the function value is decreasing, no matter to what extend.
To be able to compute the gradient of the error function in the Newton method, we switched from
error_norm
toerror_product
in the Newton. Thus, we can derive the gradient for anyerror_product
. Certainly, this restricts us to the Hilbert space case; non-Hilbert norms are no longer supported.Furthermore, we had to make some adjustments in the tests of the Newton method; the Jacobian of the
MonomOperator
s was not implemented properly. We added a test case for the line search algorithm, in which the Newton without line search starts oscillating between two values.