Repository: scipy/scipy
Pull request: ENH: Raise an error on incorrect bounds format in optimize.fmin_l_bfgs_b
<!-- 
Thanks for contributing a pull request! Please ensure that
your PR satisfies the checklist before submitting:
http://scipy.github.io/devdocs/dev/contributor/development_workflow.html#checklist-before-submitting-a-pr

Also, please name and describe your PR as you would write a
commit message:
https://docs.scipy.org/doc/numpy/dev/development_workflow.html#writing-the-commit-message

Note that we are a team of volunteers; we appreciate your
patience during the review process.

Again, thanks for contributing!
-->

#### Reference issue
<!--Example: Closes gh-WXYZ.-->
Closes gh-11659.

#### What does this implement/fix?
<!--Please explain your changes.-->
Added bounds check in fmin_l_bfgs_b to raise an error if one of the bounds is given in incorrect format. Added test to check that this error is raised. 

#### Additional information
<!--Any additional information you think is important.-->
I'd like to hear opinion of SciPy members on some matters:
Should I leave the check in fmin_l_bfgs_b or should I move it to _minimize_lbfgsb?
Is raising an error ok? Or is it better to set d[‘warnflag’] to 2 and to put the message in d[‘task’]?
Also, there is a matter of autocorrecting the bounds. 
There's an option of not correcting the bounds at all. 
Or to use a keyword argument to indicate that user wants to autocorrect them inside fmin_l_bfgs_b(or  _minimize_lbfgsb) and to not raise an error.
Or to introduce a new function that corrects bounds. 
Which one is more in SciPy spirit of doing things?

Problem statement:
optimize.fmin_l_bfgs_b needs bound check and appropiate error message
<!-- 
Thank you for taking the time to file a bug report. 
Please fill in the fields below, deleting the sections that 
don't apply to your issue. You can view the final output
by clicking the preview button above.
Note: This is a comment, and won't appear in the output.
-->

fmin_l_bfgs_b (from scipy.optimize) requires bounds as a list of bounds for each parameter/varriable given as a list [lower, upper]. If any of the bounds are given as [upper, lower] then the fmin_l_bfgs_b runs but the output dict only contains the starting position, zeros and a message saying b'No feasible solution found'. A check that the bounds are structured correctly needs to be added to fmin_l_bfgs_b and if the check is failed either the structure of the bound should be corrected or an error should be raised stating the bounds are incorrectly structured.

Hints from issue discussion:
Hello, I'd like to tackle this issue. I'm in favor of raising an exception. It will help in debugging, if user used incorrect bounds(since in documentation it's specified as `(min, max)`). Does anyone have a reason to use another approach instead? 
My preference would be for a neat way of fixing the bounds, possibly located in `_minimize.standardize_bounds`. e.g.
```
bounds = [(0, 1), (3, 2)]
limits = [(min(a, b), max(a, b) for a, b in bounds]
```
then it doesn't matter if the upper limit is less than the lower limit.


I think that doing both would be ideal. So by default an error is raised as suggested by @ruslanye and then have a keyword ('correct_bounds' for example), which defaults to False, that when is True fixes the bounds as suggested by @andyfaff. When the bounds are autocorrected then a Warning error is printed and prints out the change that has occured.
I don't think an extra keyword is desirable here, but it could be mentioned in the docs.
I think autocorrecting the issue with no warning will just change the types of issues with the bounds that get hidden. So in my problem, I had an array of values that I produced bounds from by multiplying by np.array([0.9, 1.1]). Hence for all negative values, the bounds produced were ordered (max, min). In this case optionally being able to autocorrect is ideal and it tidies up my bounds. In the case of a typo such as (4, 1), when (4, 10) was the intention, autocorrecting the bounds would cause other issues that may not be immediately obvious and might only be found down the line. 

While I like the idea of autocorrecting the bounds I think it should be optional and default to not autocorrecting and raising an exception instead. If the options are only been always autocorrecting and raising an exception then I think an exception should be raised stating the issue of incorrect bounds ordering and which bounds, or the indices of the bounds, that are incorrectly ordered.
I agree with @dgahle. I'll try to implement it as they suggested.
I'm really not a fan of adding an extra keyword here. The keyword argument doesn't add much benefit, and makes the list of arguments longer.

The two realistic options are either

1. Just raise a RuntimeError if the min/max aren't in the correct order

-or-

2. Autocorrect the bounds for min/max and raise a RuntimeWarning.
Well, the keyword argument will have a default value, so the list of arguments will be longer only if you use the option to correct bounds. Alternatively, we can always raise an error and add a function `_minimize.standardize_bounds`, as you mentioned, so users can correct bounds without using a keyword.

Commit message: ENH: Raise an error on incorrect bounds format in optimize.fmin_l_bfgs_b

Patch to review:
diff --git a/scipy/optimize/lbfgsb.py b/scipy/optimize/lbfgsb.py
index 87b1a5251276..5925136e9814 100644
--- a/scipy/optimize/lbfgsb.py
+++ b/scipy/optimize/lbfgsb.py
@@ -169,6 +169,12 @@ def fmin_l_bfgs_b(func, x0, fprime=None, args=(),
       ACM Transactions on Mathematical Software, 38, 1.
 
     """
+    # check bounds
+    if bounds is not None:
+        for lb, ub in bounds:
+            if lb is not None and ub is not None and lb > ub:
+                raise ValueError('bounds should be given as (min, max) pairs.')
+    
     # handle fprime/approx_grad
     if approx_grad:
         fun = func
diff --git a/scipy/optimize/tests/test_optimize.py b/scipy/optimize/tests/test_optimize.py
index 9cfcbb604bb4..5299ad0d4966 100644
--- a/scipy/optimize/tests/test_optimize.py
+++ b/scipy/optimize/tests/test_optimize.py
@@ -1145,6 +1145,17 @@ def test_l_bfgs_b_bounds(self):
         assert_(d['warnflag'] == 0, d['task'])
         assert_allclose(x, self.solution, atol=1e-6)
 
+    @pytest.mark.parametrize('bounds', [
+        ([(10, 1), (1, 10)]),
+        ([(1, 10), (10, 1)]),
+        ([(10, 1), (10, 1)])
+    ])
+    def test_l_bfgs_b_wrong_bounds(self, bounds):
+        with pytest.raises(ValueError, match='.*bounds.*'):
+            optimize.fmin_l_bfgs_b(self.fun, [0, -1],
+                                         fprime=self.jac,
+                                         bounds=bounds)
+
     def test_l_bfgs_b_funjac(self):
         # L-BFGS-B with fun and jac combined and extra arguments
         x, f, d = optimize.fmin_l_bfgs_b(self.fj, [0, -1], args=(2.0, ),


Code review comments:
diff_hunk: @@ -1145,6 +1145,17 @@ def test_l_bfgs_b_bounds(self):
         assert_(d['warnflag'] == 0, d['task'])
         assert_allclose(x, self.solution, atol=1e-6)
 
+    @pytest.mark.parametrize('bounds', [
+        ([(10, 1), (1, 10)]),
+        ([(1, 10), (10, 1)]),
+        ([(10, 1), (10, 1)])
+    ])
+    def test_l_bfgs_b_wrong_bounds(self, bounds):
+        with pytest.raises(ValueError, match='.*bounds.*'):
+            optimize.fmin_l_bfgs_b(self.fun, [0, -1],
path: scipy/optimize/tests/test_optimize.py
text: @user1:
Please can you do the test using `optimize.minimize` using the `L-BFGS-B` method rather than `fmin_l_bfgs_b` function? 
i.e. in a similar fashion to `test_minimize_l_bfgs_b_bounds` a few lines below.