Problems setting basinhopping optimizer

[Muammar El Khatib]

In our documentation it is stated the following:

To change how the code manages the regression process, you can use the Regressor class. For example, to switch from the scipy’s fmin_bfgs optimizer (the default) to scipy’s basin hopping optimizer, try inserting the following lines before initializing training:

from amp.regression import Regressor
from scipy.optimize import basinhopping

regressor = Regressor(optimizer=basinhopping)
calc.model.regressor = regressor

As I wanted to use other optimizer, I tried basinhopping in the test_gaussian_neural.py following the readthedocs website. I modified it as it is seen in the diff below:

diff --git a/tests/test_gaussian_neural.py b/tests/test_gaussian_neural.py
index 9a2d570..81ae3a0 100644
--- a/tests/test_gaussian_neural.py
+++ b/tests/test_gaussian_neural.py
@@ -16,6 +16,9 @@ from amp.descriptor.gaussian import Gaussian
 from amp.model.neuralnetwork import NeuralNetwork
 from amp.model import LossFunction

+from amp.regression import Regressor
+from scipy.optimize import basinhopping
+

 def generate_data(count):
     """Generates test or training data with a simple MD simulation."""
@@ -48,6 +51,8 @@ def train_test():
                model=NeuralNetwork(hiddenlayers=(3, 3)),
                label=label,
                cores=1)
+    regressor = Regressor(optimizer=basinhopping)
+    calc.model.regressor = regressor
     loss = LossFunction(convergence={'energy_rmse': 0.02,
                                      'force_rmse': 0.02})
     calc.model.lossfunction = loss

Executing the script, gives back the following error:

~/git/ase/ase/lattice/surface.py:17: UserWarning: Moved to ase.build
  warnings.warn('Moved to ase.build')
Traceback (most recent call last):
  File "test_gaussian_neural.py", line 67, in <module>
    train_test()
  File "test_gaussian_neural.py", line 60, in train_test
    calc.train(images=train_images,)
  File "~/git/amp/amp/__init__.py", line 308, in train
    parallel=self._parallel)
  File "~/git/amp/amp/model/neuralnetwork.py", line 221, in fit
    result = self.regressor.regress(model=self, log=log)
  File "~/git/amp/amp/regression/__init__.py", line 89, in regress
    **self.optimizer_kwargs)
  File "/home/muammar/.local/lib/python2.7/site-packages/scipy/optimize/_basinhopping.py", line 602, in basinhopping
    niter_success = niter + 2
TypeError: unsupported operand type(s) for +: 'instancemethod' and 'int'

Looking at line 602 the problem is that niter is not being passed as an argument when calling the basinhopping method. At this point, I asked myself, what am I doing wrong?. As I didn't find a way of calling this correctly, I started to look inside regression/__init__.py and I came with the following patch:

diff --git a/amp/regression/__init__.py b/amp/regression/__init__.py
index 8da9d7f..205daaa 100644
--- a/amp/regression/__init__.py
+++ b/amp/regression/__init__.py
@@ -56,6 +56,9 @@ class Regressor:
         elif optimizer == 'NCG':
             from scipy.optimize import fmin_ncg as optimizer
             optimizer_kwargs = {'avextol': 1e-15, }
+        elif optimizer == 'basinhopping':
+            from scipy.optimize import basinhopping as optimizer
+            optimizer_kwargs = {}

         if user_kwargs:
             optimizer_kwargs.update(user_kwargs)
@@ -84,9 +87,11 @@ class Regressor:
         log(' Optimizer kwargs: %s' % self.optimizer_kwargs)
         x0 = model.vector.copy()
         try:
-            if self.lossprime:
+            if self.lossprime is True and self.optimizer.__name__ != 'basinhopping':
                 self.optimizer(model.get_loss, x0, model.get_lossprime,
                                **self.optimizer_kwargs)
+            elif self.lossprime is True and self.optimizer.__name__ == 'basinhopping':
+                self.optimizer(model.get_loss, x0, **self.optimizer_kwargs)
             else:
                 self.optimizer(model.get_loss, x0, **self.optimizer_kwargs)

I realized that when self.lossprime is True basinhopping method cannot be called as self.optimizer(model.get_loss, x0, model.get_lossprime, **self.optimizer_kwargs). Applying that patch makes possible to run bassinhopping optimizer and test_gaussian_neural.py looks as follows:

diff --git a/tests/test_gaussian_neural.py b/tests/test_gaussian_neural.py
index 9a2d570..a012c60 100644
--- a/tests/test_gaussian_neural.py
+++ b/tests/test_gaussian_neural.py
@@ -16,6 +16,7 @@ from amp.descriptor.gaussian import Gaussian
 from amp.model.neuralnetwork import NeuralNetwork
 from amp.model import LossFunction

+from amp.regression import Regressor

 def generate_data(count):
     """Generates test or training data with a simple MD simulation."""
@@ -48,6 +49,8 @@ def train_test():
                model=NeuralNetwork(hiddenlayers=(3, 3)),
                label=label,
                cores=1)
+    regressor = Regressor(optimizer='basinhopping')
+    calc.model.regressor = regressor
     loss = LossFunction(convergence={'energy_rmse': 0.02,
                                      'force_rmse': 0.02})
     calc.model.lossfunction = loss

It gives the following results:

~/git/ase/ase/lattice/surface.py:17: UserWarning: Moved to ase.build
  warnings.warn('Moved to ase.build')
energy = 9.39423340031
forces = [[ -1.228375030217603164350848032882e-01
   -7.687963073710024630713633086998e-02
    1.668923308381203618466770421946e+00]
 [  1.228375030217603858240238423605e-01
   -7.687963073710014916262167616878e-02
    1.668923308381203174377560571884e+00]
 [ -1.228375030217602748017213798448e-01
    7.687963073709991324022894332302e-02
    1.668923308381203174377560571884e+00]
 [  1.228375030217603303128726111026e-01
    7.687963073709980221792648080736e-02
    1.668923308381203618466770421946e+00]
 [ -2.501656687582918859063156200527e-16
    4.310383749427206205635143634881e-17
   -1.730500194479561049121230098535e+00]
 [  1.942890293094023945741355419159e-16
    4.328546123830906961908271525665e-17
   -1.609562152906351295555964497908e+00]
 [ -3.842943174052633670823905642616e-16
    2.654968934984854978158860985892e-16
   -1.742304416310902492881496073096e+00]
 [  2.025879336683987369712975701730e-17
    2.859502553192889818145861355956e-16
   -7.416450144871182814654275716748e-01]
 [  1.040834085586084256647154688835e-16
   -2.224192392571321132518634896227e-16
    8.206489919808934718759019233403e-01]
 [ -2.093050299573485207162449954467e-21
    2.425842304008389289470443800012e-16
   -1.672330447321773938540445669787e+00]]
energy = 8.3835181675
forces = [[-0.078393157452713663202459315471 -0.117221906875157849681201582825
   1.131566894306427206728926648793]
 [ 0.408441588535414956684377330021 -0.309112642979070373350225509057
   0.649799390761325756926680696779]
 [-0.049611195075230485329598195676  0.123547217461742742106167725069
   1.114069518738374009458880209422]
 [ 0.090749604039490783247146055146  0.161353295364937893152657011342
   0.782184980261523166156223396683]
 [ 0.204065968683720094789535437485  0.039985350682295271729227437163
  -1.177327586815578364110024267575]
 [-0.149506518112411473930833949453 -0.002937297184445846788547473949
  -0.860911202570738831596486306807]
 [ 0.046463731740455649354970546483 -0.06640675283504883508189919894
  -0.80985299019553536048476871656 ]
 [-0.513187948203867150276380471041  0.170837321755992088689879437879
  -0.314783915203934450133260725124]
 [ 0.039670890740397743434719757261  0.032930239685396787974180909941
  -0.481248715174550645290452166591]
 [ 0.001307035104743603558599684966 -0.032974825076641914312070014148
  -0.033496374107312598678021231535]]

Which are comparable to fmin_l_bfgs_b:

energy = 9.39117278516
forces = [[ -1.153434483683473954496179203488e-01
   -5.818387385625473084971304160717e-02
    1.654904163399040006865448049211e+00]
 [  1.153434483683473538162544969055e-01
   -5.818387385625475860528865723609e-02
    1.654904163399039784820843124180e+00]
 [ -1.153434483683474232051935359777e-01
    5.818387385625437002723003843130e-02
    1.654904163399040228910052974243e+00]
 [  1.153434483683472566717398422043e-01
    5.818387385625443247727517359635e-02
    1.654904163399040006865448049211e+00]
 [  5.272793030631735233443214728476e-18
    2.451237312589942922932557392214e-16
   -1.723964031998213020102639347897e+00]
 [ -5.551115123125782702118158340454e-17
    2.553525859685849414134734019389e-16
   -1.621265532252126106982359488029e+00]
 [ -1.639546269043846613798762969386e-16
    7.895699105081839442863133322225e-17
   -1.718128890831999155963671910285e+00]
 [  3.203958524968352350739401637794e-17
    8.772884088520568021722231770381e-17
   -7.280210060699792151694964559283e-01]
 [  5.204170427930421283235773444176e-18
   -6.415614873263600736860536347094e-17
    8.271398569860934912867378443480e-01]
 [ -2.989150293546754549132166029012e-21
    7.062988811220904820568965637736e-17
   -1.655377049429935798485757914023e+00]]
energy = 8.82911201125
forces = [[-0.201061051046554639665231434265 -0.022783857194286896707957623676
   1.452320061249322780128068188787]
 [ 0.29791244229884844552103118076   0.031940138386208696830159681213
   1.198328329515925050330338308413]
 [-0.224293633130754066495882170784  0.038336683890834166721361953023
   1.454233330647161759330288077763]
 [ 0.253641688987920832509814772493  0.037872312532792100359113618424
   1.246875577712129867791190918069]
 [ 0.073583501729309747085849835457 -0.050255283551288942867074638343
  -1.53110374751293054629286416457 ]
 [-0.159084661015692352403760878587 -0.029813359443213064553512836596
  -1.049243002626313314706862911407]
 [ 0.038541360786699632723362185516  0.039192421780616035209554581797
  -1.467815530230767606667541258503]
 [-0.07178563321558593945947990278  -0.038602001768938479231429994343
  -0.532352956547200051318213809282]
 [ 0.037922008657323383329362798122 -0.01085801167806331110621265168
  -0.02737603695750259746866106525 ]
 [-0.045376024051515022328384674211  0.004970957045339517536841622558
  -0.743866025249825590925922824681]]

Before proceeding to commit/push to master, I would like to know if it is not me who is having an error elsewhere in the input file or if there is another way to pass niter when calling this method.

Thanks.

[Muammar El Khatib]

I forgot to mention that lossprime being a boolean one could set it to be False but still there would be a limitation when using basinhopping.

[andrew_peterson]

Ok -- let's talk in person a bit. I guess the root problem is that some scipy optimizers take slightly different syntaxes? Another way might be that we have a standard optimizer syntax, then the user needs to wrap the optimizer they want to use in order to have the same syntax. That would create fewer "if" statements within our code as we try to look for all these optimizer versions...

[Muammar El Khatib]

I like the idea of having a standardized optimizer syntax within Amp, because as you mentioned, it would avoid having many "if" statements in the code. I look forward to discussing it.

[andrew_peterson]

So I think this is just a matter of updating the documentation to tell the user they need to wrap their optimizer to look like fmin_bfgs. I guess this is already stated correctly in the docstrings, just not the website. @muammar , can you take care of this?

[Muammar El Khatib]

I will take care of this @andrewpeterson !

[Alireza Khorshidi]

Another way is to add basinhopping to the list here and at the same time turn the lossprime off, by something like

elif optimizer == 'NCG':
    from scipy.optimize import fmin_ncg as optimizer
    optimizer_kwargs = {'avextol': 1e-15, }
elif optimizer == 'basinhopping':
    from scipy.optimize import basinhopping as optimizer
    optimizer_kwargs = {...}
    self.lossprime = False

[andrew_peterson]

That would also be fine.

But I still like the idea of just demanding that the user wrap whatever optimizer they use in order to take the same keywords as fmin. Then we don't have to keep up with any changes at scipy, and its more apparent that the users could in principle pull an optimizer from anywhere (not just a pre-defined list).

[Muammar El Khatib]

Updating documentation for setting optimizers. This closes #127.

→ <<cset a49e892032a4>>

[andrew_peterson]

Scipy has a new "minimize" function at scipy.optimize.minimize. We should probably switch to expect that. (Unless it creates backward compatibility issues with users with older versions of scipy.) See here this comment from amp-users:

"""Part of the issue was that all of the scipy optimizers have slightly different interfaces; beyond the necessary differences (like no derivatives is necessary for basin hopping, so no fprime keyword), other keywords had different names in different optimizers. For example, fmin_bfgs uses "f" for the callable function, while basinhopping uses "func". Our solution to this was to just ram in positional arguments, rather then keyword arguments, which is a little ugly and can contribute to errors like these. But anyway, scipy has now introduced a function called scipy.optimize.minimize which is a uniform interface to all these optimizers. We should probably switch our code to expect that interface instead, which should make things easier. I'll open a ticket."""

[Muammar El Khatib]

Merge branch 'master' into krr_param

• master: Updating documentation for setting optimizers. This closes #127.

→ <<cset edc5c5dd3122>>

[Muammar El Khatib]

Merge branch 'master' into krr_param

• master: Updating documentation for setting optimizers. This closes #127.

→ <<cset edc5c5dd3122>>