Stefan-Endres / shgo

Simplicial Homology Global Optimization
https://stefan-endres.github.io/shgo
MIT License
44 stars 12 forks source link

Replace check-and-update pattern with dict.update #12

Closed alchemyst closed 3 years ago

alchemyst commented 6 years ago

Instead of the pattern of

if options is not None:
    if 'f_tol' in options:
        self.minimizer_kwargs['options']['ftol'] = options['f_tol']
...

I think we can rather do

if options is not None:
    self.minimizer_kwargs.update(options)

This will automatically overwrite keys if they exist in options. Less code to maintain.

Stefan-Endres commented 6 years ago

Ok I'll work on this issue.

Stefan-Endres commented 6 years ago

I'm undoing this change because it's causing a lot of issues with unknown keyword arguments raising warnings and failing tests in modules that raise warnings to a failure.

If we want to use this we will need to write code to check every single function wrapped as a method by the scipy.optimize.minimize function. Then pop the unknown options for the minimizer_kwargs dict:

https://stackoverflow.com/questions/11277432/how-to-remove-a-key-from-a-python-dictionary

In my opinion this is far worse to maintain than just adding the keywords to specific solvers. But I will reopen this issue to discuss it if there is a better option.

alchemyst commented 6 years ago

I understand, if we only want to copy the particular keys we use, I suggest we start the options with a default for every key we use like

self.minimizer_kwargs = {'ftol': 1}

(which we can see from the ifs) and then introduce this functionL

def update_if_exists(dicta, dictb):
    if dictb is None:
        return
    shared_keys = set(dicta).intersection(dictb)
    for k in shared_keys:
        dicta[k] = dictb[k]

Then we can replace the whole

if options is not None:
    self.minimizer_kwargs.update(options)

with

update_if_exists(self.minimizer_kwargs, options)

I didn't take into account that this code would be called by other code which may pass extra options.

If we want to raise a warning ourselves about unknown arguments, that function would also make a nice place to do it.