Fix optimize defaults for older scipy versions for L-BFGS-B #476
Conversation
| @@ -104,7 +99,7 @@ def __init__(self, fun, x0, args=(), method='L-BFGS-B', jac=None, | |||
|
|
|||
| callback : callable, optional | |||
| Called after each iteration, as ``callback(xk)``, where ``xk`` is | |||
| the current parameter vector. | |||
| the current parameter vector. Only available using Scipy 0.12+. | |||
There was a problem hiding this comment.
Small nit: I'd use "scipy > 0.12", rather than "0.12+"
|
General thought: with the new Travis config, should we setup up CI testing for both scipy<0.12 and for scipy>=0.12? We already have 6 machines tested there... |
|
@arokem I changed the 0.12+ to >=0.12 As about setting up CI testing I think it would be great to have a range of different numpy and scipy versions. So be my guest. Go forward with merging this. And will push in a bit the other one with the enhancement of SLR to support previous transformations. |
|
Sure. Will wait for Travis to finish and then merge this in. More changes On Sun, Nov 23, 2014 at 2:29 PM, Eleftherios Garyfallidis <
|
|
Travis has finished. GGT! |
Fix optimize defaults for older scipy versions for L-BFGS-B
|
Yeah - I am not very happy with this - test coverage on this is very poor On Sun, Nov 23, 2014 at 2:49 PM, Eleftherios Garyfallidis <
|
| self._evol_kx = None | ||
|
|
||
| _eps = np.finfo(float).eps |
There was a problem hiding this comment.
why is eps changed to the lowest possible value? you might get convergence issues at line 155 for example, and anywhere else it's used.
|
Really - have we merged a file with 40% coverage? |
|
It will never be much more than that. If scipy<0.12 it runs one block of On Sun, Nov 23, 2014 at 3:02 PM, Matthew Brett notifications@github.com
|
|
Hey wait a minute guys. The coverage is low because it depends on which version of scipy you are running. |
|
I'm really wondering about the tolerance set at the bare floating point minimum though. |
|
Otherwise we need to separate the tests into two functions and skip the tests with the different scipy versions. This is an alternative. |
|
yep - we're having two conversations here ... On Sun, Nov 23, 2014 at 3:05 PM, Samuel St-Jean notifications@github.com
|
|
now it's three - including the meta-conversation I just started :-) On Sun, Nov 23, 2014 at 3:05 PM, Ariel Rokem arokem@gmail.com wrote:
|
|
I don't think that would solve the problem - there would still be low On Sun, Nov 23, 2014 at 3:05 PM, Eleftherios Garyfallidis <
|
|
Okay look I will make another PR using the skip mechanism. Maybe that is better and will not reduce the coverage. Let me give it a try. |
|
As for On Sun, Nov 23, 2014 at 3:05 PM, Samuel St-Jean notifications@github.com
|
|
Maybe use Travis to test on both versions of scipy, and compare the On Sun, Nov 23, 2014 at 3:10 PM, Eleftherios Garyfallidis <
|
|
Okay no. The coverage will be small no matter what I do. I don't see how I can increase the coverage here. This is truly conditional here on the scipy version that you have on your machine. If you don't have scipy >= a large part of the code will not be tested. And the opposite. |
|
@arokem with what scipy version was the coverage 40%? Do you remember? I cannot see Travis results anymore. |
|
If you don't remember. No worries. |
|
It didn't run lines 130-210, so must be >=0.12 On Sun, Nov 23, 2014 at 3:33 PM, Eleftherios Garyfallidis <
|
|
I don't see how factr is related to the number of iterations, according to the doc at http://docs.scipy.org/doc/scipy-0.14.0/reference/generated/scipy.optimize.fmin_l_bfgs_b.html it's on a way overkill level that will never converge/take forever. By default, Converge if <= factr * eps, with the doc saying : Typical values for factr are: 1e12 for low accuracy; 1e7 for moderate accuracy; 10.0 for extremely high accuracy. Currently it stands at ftol = 1e-7. Default scipy wil be factr=10000000.0 * 1e-8 = 0.1, and if they use eps=1e-16 on 64 bits system (do they actually? doc says eps is the machine precision, so that's not really clear) factr=1e-09. Is there any reason to change those defaults? And the number of maxiteration is already controlled by another variable, this is for the relative convergence of the cost function rather. |
|
@samuelstjean you are looking at the wrong scipy version dude! Please be more careful with your comments. |
|
The default value of the On Sun, Nov 23, 2014 at 3:36 PM, Samuel St-Jean notifications@github.com
|
|
OK - turns out we did have a machine running the minimal versions of the On Sun, Nov 23, 2014 at 3:31 PM, Eleftherios Garyfallidis <
|
|
What do you mean, this branch of code I mentionned is entered if scipy < 0.12 is it not? Still exists on 0.14, and they seem to have added a maxiter argument as the only difference. Anyway, I still don't see how factr and the number of iterations are related nor should be changed. |
|
I didn't invent this by myself I saw what they do in minimize to call fmin_lbfgs_b with a common interface of parameters. I did this sometime ago. I will check again just in case I missed something. |
|
Well, it's just that according to the doc, setting factr as it is will give 1e-7, which means the function will only stop is the relative difference between two evaluation fo the cost function is lower than 1e-7. They also list 10 as being of extreme precision, so 1e-7 is way overkill and should probably never converge for this criterion, only to hit the maximum number of iterations everytime instead. |
|
Okay, can you make some suggestions for default options which you think they will be good? |
|
Using the default they are using. If they picked that, there is probably a good reason, so why change it after all. As it stands it's |
|
When we tried the default values with the streamline-based registration they didn't work. And the cost function was very simple. Anyway will have a look again. But let me make something clear. At this stage I am not trying to make a general Optimizer class we don't have enough user cases yet to design something like that. What I am trying to do is make a simple class that deals with different version of Scipy. Hopefully, when we switch to Scipy 0.11 or 0.12 we can forget about all these and just use the wrapper function minimize which simplifies playing with different parameters across different optimizers. |
|
Well, it was designed as a general optimization wrapper for starters, so it largely depends on the cost function used. If you use it on anything least-square based with large numbers (like registration of images based on intensities) it is way too high I'd say. As for specilalized application like the bundle registration, you might right away reach the convergence criterion with those same values. It's hard to set defaults as the use case might be largely different of course, so there is no single answer here. |
Enabling Optimizer class to work with < 0.12 versions of Scipy for L-BFGS-B can be tricky. The parameters of L-BFGS-B function changed from 0.11 to 0.12. Here is a workaround. I also added some more prints reporting what is actually happening when < 0.12. GGT! @arokem release the Kraken!