Skip to content
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

BUG fixed printing of convergence message in minimize_scalar in scipy/optimize #8273

Merged
merged 3 commits into from
Jan 11, 2018

Conversation

chrisb83
Copy link
Member

@chrisb83 chrisb83 commented Jan 8, 2018

to display convergence messages, call minimize_scalar with disp=True. However, if method='Bounded', it does not print messages as condition is "if disp > 2" instead of "if disp". fixed in this commit (function
_minimize_scalar_bounded)

Example before my commit:

f = lambda x: x**2
optimize.minimize_scalar(f, bounds=((2, 3)), method='Bounded', options={'disp': True}) # does not print
optimize.minimize_scalar(f, bounds=((2, 3)), method='Bounded', options={'disp': 2.5}) # does print convergence messages

…/optimize

modified:   scipy/optimize/optimize.py

to display convergence messages, call minimize_scalar with disp=True.
However, if method='Bounded', it does not print messages as condition
is "if disp > 2" instead of "if disp". fixed in this commit (function
_minimize_scalar_bounded)
Copy link
Member

@dlax dlax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution! As noted in the inline comment, I think this is not the correct approach, but there's a way forward.

xatol=1e-5, maxiter=500, disp=0,
**unknown_options):
def _minimize_scalar_bounded(func, bounds, args=(), xatol=1e-5, maxiter=500,
disp=False, **unknown_options):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, this would break how verbosity is controlled in fminbound() which also uses this function (though, perhaps there's no test coverage for this).
So disp should remain an integer here (and changes below be reverted). Instead, minimize() should be fixed to call _minimize_scalar_bounded() with disp as an integer obtained from the bool value it receives. For instance:

diff --git a/scipy/optimize/_minimize.py b/scipy/optimize/_minimize.py
--- a/scipy/optimize/_minimize.py
+++ b/scipy/optimize/_minimize.py
@@ -655,6 +655,11 @@ def minimize_scalar(fun, bracket=None, bounds=None, args=(),
         if bounds is None:
             raise ValueError('The `bounds` parameter is mandatory for '
                              'method `bounded`.')
+        # replace boolean "disp" option, if specified, by an integer value, as
+        # expected by _minimize_scalar_bounded()
+        disp = options.get('disp')
+        if isinstance(disp, bool):
+            options['disp'] = 2 * int(disp)
         return _minimize_scalar_bounded(fun, bounds, args, **options)
     elif meth == 'golden':
         return _minimize_scalar_golden(fun, bracket, args, **options)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing this out, I reverted the commit and changed the code as suggested. I also corrected the docstring in _minimize_scalar_bounded (now stating that disp is int instead of bool).

@dlax dlax added defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.optimize labels Jan 8, 2018
….optimize

followed suggestion by dlax in scipy#8273 and converted disp from bool to int
also corrected docstring of _minimize_scalar_bounded (disp is int instead of bool)
@dlax dlax merged commit a03fd3f into scipy:master Jan 11, 2018
@chrisb83 chrisb83 deleted the bugfix_optimize_scalar branch March 25, 2018 20:04
@chrisb83
Copy link
Member Author

@dlax Hi, does this PR need to be still need to be scheduled for a release? (It was not included in 1.0.1) Thanks

@ilayn
Copy link
Member

ilayn commented Mar 25, 2018

It is included. It just didn't have the milestone set. v1.0.1 is branched later than this commit

Oh wait, scratch that I was comparing to master yes it needs to go in.

@ilayn ilayn added this to the 1.1.0 milestone Mar 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.optimize
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants