BUG: Add test for Stackoverflow example from issue 8174. #9096

Kai-Striega · 2018-07-31T04:13:45Z

Test of the stackoverflow issue in issue #8174 as suggested by @mdhaber. The result returned from linprog(method='simplex') now matches that of linprog(method='interior-point'):

import numpy as np
from scipy.optimize import linprog
c = np.array([1,0,0,0,0,0,0])
A_ub = np.identity(7)*(-1)
b_ub = np.array([[-2],[-2],[-2],[-2],[-2],[-2],[-2]])
A_eq = np.array([[1,1,1,1,1,1,0],[0.3,1.3,0.9,0,0,0,-1],[0.3,0,0,0,0,0,-2/3],
                 [0,0.65,0,0,0,0,-1/15],[0,0,0.3,0,0,0,-1/15]])
b_eq = np.array([[100],[0],[0],[0],[0]])
res = linprog(c = c, A_ub=A_ub, b_ub=b_ub, A_eq = A_eq, b_eq = b_eq)
print('Simplex method:')
print(res)

print()
print('Interior-Point method')
res_ip = linprog(c = c, A_ub=A_ub, b_ub=b_ub, A_eq = A_eq, b_eq = b_eq, method='interior-point')
print(res_ip)

Simplex method:
     fun: 43.33333333333336
 message: 'Optimization terminated successfully.'
     nit: 15
   slack: array([41.33333333,  0.        ,  2.33333333, 44.33333333,  0.        ,
        0.        , 17.5       ])
  status: 0
 success: True
       x: array([43.33333333,  2.        ,  4.33333333, 46.33333333,  2.        ,
        2.        , 19.5       ])

Interior-Point method
     con: array([ 2.79356982e-10, -1.52451385e-11,  3.71258579e-12, -5.92437210e-12,
       -2.36921593e-12])
     fun: 43.33333333313849
 message: 'Optimization terminated successfully.'
     nit: 5
   slack: array([4.13333333e+01, 6.92779167e-13, 2.33333333e+00, 1.47777778e+01,
       1.47777778e+01, 1.47777778e+01, 1.75000000e+01])
  status: 0
 success: True
       x: array([43.33333333,  2.        ,  4.33333333, 16.77777778, 16.77777778,
       16.77777778, 19.5       ])

mdhaber · 2018-07-31T07:54:10Z

Huh. Well that's convenient. Do you think #8909 fixed this?

Kai-Striega · 2018-07-31T09:01:19Z

I guess so? #8909 implemented the only major changes to actually help fix bugs rather than to educate users. I must admit, I hadn't actually run a test on that particular issue.

Kai-Striega · 2018-08-02T04:18:13Z

@rgommers, @mdhaber Can we merge this and close #8174?

mdhaber

Functionally looks good. You appear to have copy-pasted the example code by @sschnug from #8174 (good) and made minor formatting tweaks; and the required objective value is the same as what IPM reported then, so it seems to be the intended test. In addition to requiring that objective value match, you require that presolve catch the row redundancy. Just two minor suggestions.

mdhaber · 2018-08-02T04:58:16Z

scipy/optimize/tests/test_linprog.py

+        ])
+        b_eq = np.array([[100],[0],[0],[0],[0]])
+
+        shoud_warn_A_not_full_rank = (


misspelled should here and below : )

mdhaber · 2018-08-02T05:05:25Z

scipy/optimize/tests/test_linprog.py

+
+        shoud_warn_A_not_full_rank = (
+            self.options.get('presolve', True)
+            and self.method == 'interior-point'


Because only interior point currently implements presolve, this is redundant with the line above, right? And when simplex does implement presolve, we would want it to be subject to the same condition. So I suggest removing this second condition.

pytest.warns(...) fails the test if no warning is raised. Removing the second condition will cause the tests to fail because the simplex method does not implment presolve (yet).

@Kai-Striega Maybe I'm wrong, but I thought currently simplex is never passed a presolve option at all. So the first condition self.options.get('presolve', True) will always be False for simplex tests. Because and is a short circuit operator, the second condition checking for interior point will never even be evaluated, so it is redundant. shoud_warn_A_not_full_rank will be false, so the pytest.warns part won't apply to simplex tests, so it wouldn't cause simplex tests to fail.

Please forgive me if I'm being dense! What part of this is wrong?

Actually... I don't think presolve is ever going to be in self.options for this test, even for interior point. So I think that should_warn_A_not_full_rank is never True. Are you sure that the with pytest.warns(OptimizeWarning) is ever reached?

Since presolve is never in self.options, you don't really need to check it at all. Since presolve is on for IPM by default, I think what you want to do is something like:

if method = "interior point": # run linprog, checking for OptimizeWarning else # run linprog normally _assert_success(...)

@mdhaber Yes you're right. I'll change this test. I was trying to leave this open incase we end up adding any tests without using presolve.

Yes you're right. I'll change the test. I wasn't sure if there are any tests for problems without presolve and wanted to be able to accomodate those. I'm pretty sure that, mathematically, every problem should return the same result regardless of whether presolve is used.

mdhaber · 2018-08-02T05:16:42Z

@Kai-Striega Please consider the minor comments, then when you're done ping me again and I'll merge this.

I don't think this quite allows us to close #8174 because doesn't the first code example from #8174 still return success incorrectly? Even though simplex now warns the user, it still needs to return an error code when constraints aren't satisfied - please see my last comment in #9076. Anyway, I believe your next planned PR was to use the IPM pre- and post- routines, and that will fix this, allowing us to close #8174 and several other issues. Very exciting!

…_8174_so

…cipy into test_issue_8174_so

mdhaber · 2018-08-03T04:48:45Z

scipy/optimize/tests/test_linprog.py

+                c=c, A_ub=A_ub, b_ub=b_ub, A_eq=A_eq, b_eq=b_eq,
+                method=self.method, options=self.options
+            )
+            _assert_success(res, desired_fun=43.3333333331385)


@Kai-Striega Why not just one of these outside the if-elif?
But I suppose If I see the tests pass tonight I'll merge.

If added this using if-elseif to avoid having the test run if an invalid test is run.
However having the test run outside of the if block raises an unbound local error.

I don't understand what you mean by:
"If added this using if-elseif to avoid having the test run if an invalid test is run."

It sounds like by "invalid test" you mean if self.method takes on some value other than simplex or interior-point. That will not happen until we add a new method, and then we'll have to review this test anyway. It would probably be better if this caused the test to fail, actually, because it reminds us that we need to reconsider how this test should be applied for the new method.

Also, we're going to adding presolve to simplex soon, right? So soon enough, we'll actually want to take out the if/elif altogether, because the test should be applied to both methods the same way. I don't anticipate that we'll be adding any new methods before then.

Whether you use this if-elif or if-else like I suggest, res is guaranteed to be defined as the tests are currently written, so I don't see the harm of putting a single _assert_success outside these blocks.

I don't see how an UnboundLocalError is relevant, as no assignments are made to res (see here for an explanation UnboundLocalError).

I suggest trying my suggestion out. If you do get some error, please let me know, but if the tests pass with a single _assert_success outside these blocks, I think that's how it should be.

mdhaber · 2018-08-03T04:50:04Z

scipy/optimize/tests/test_linprog.py

+                )
+            _assert_success(res, desired_fun=43.3333333331385)
+
+        elif self.method == 'interior-point':


@Kai-Striega Shouldn't this just be an else? In any case, certainly not self.method == 'interior-point'!

I've avoided else incase a different variable is added. This is probably overkill / or should raise a valuerror instead.

@Kai-Striega I'm not sure what you mean by "incase a different variable is added". Do you mean if a different value for method is added?

As the test is written, if a different value for method is added, then this test will effectively not run at all, because neither the if or elif code block will run.

In fact, even if the method is simplex, neither of these blocks will run because you have if self.method == 'interior-point': and elif self.method == 'interior-point':. If you really think the elif is appropriate, shouldn't it at least be elif self.method == 'simplex':?

I don't understand the part about raising a ValueError.

I don't think it's necessary to anticipate additions of other methods right now, as all tests that have method-dependent requirements will have to be reconsidered if we add a new method.

Sorry this has been a mental short circuit on my part. I'm going to change it to your suggestion and fix the logic (again).

mdhaber · 2018-08-04T03:58:26Z

scipy/optimize/tests/test_linprog.py

@@ -820,20 +820,18 @@ def test_issue_8174_stackoverflow(self):

        if self.method == 'interior-point':
            # Warning is raised in presolve method,
-            # which is not yet implemented in the simplex method.
+            # which is not hyet implemented in the simplex method.


mdhaber · 2018-08-04T03:58:43Z

scipy/optimize/tests/test_linprog.py

@@ -797,10 +797,10 @@ def test_bug_5400(self):
            with pytest.warns(OptimizeWarning):
                res = linprog(c, A_ub, b_ub, bounds=bounds,
                    method=self.method, options=self.options)
-        elif self.method == 'interior-point':
+        else self.method == 'interior-point':


mdhaber · 2018-08-04T04:00:18Z

scipy/optimize/tests/test_linprog.py

-            _assert_success(res, desired_fun=43.3333333331385)
-
-        elif self.method == 'interior-point':
+        else self.method == 'simplex':


Kai-Striega · 2018-08-04T05:04:12Z

Sorry. This was supposed to be a quick test and it’s turning fast turning into an embarrassment for me. Have a look and check if anything is better, otherwise I’m going to let it sit for a few days and come back to it.

mdhaber · 2018-08-04T05:06:02Z

@Kai-Striega I think you've got it! Fingers crossed that the tests pass : ) Thanks for your patience with this.

Kai-Striega · 2018-08-04T07:46:33Z

The failing test comes from:

$ python -c 'import mpmath.libmp; assert mpmath.libmp.BACKEND == "gmpy"'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
AssertionError

Which seems to be a problem for quite a few PRs right now (see #9106).Once that’s fixed it should be good to go.

Kai-Striega · 2018-08-09T11:20:29Z

@mdhaber can we merge this? All the tests pass, except those on OSX. The same error occurs for most (all?) of the current PRs.

mdhaber · 2018-08-09T14:06:16Z

I know. I thought it would be ok to wait for the macOS issue to be resolved and get a clean bill of health.
@rgommers can we merge or should we wait?

rgommers · 2018-08-09T15:38:21Z

please go ahead and merge. no point in waiting

Add test for Stackoverflow example from issue 8174.

0093e69

Kai-Striega mentioned this pull request Jul 31, 2018

linprog failure on provided input #8174

Closed

mdhaber reviewed Aug 2, 2018

View reviewed changes

Fix spelling error in variable name.

850f8b1

pvanmulbregt added the scipy.optimize label Aug 2, 2018

Kai-Striega added 3 commits August 3, 2018 12:28

Fix check for presolve and spelling error in variable name.

279257d

Merge branch 'master' of git://github.com/scipy/scipy into test_issue…

51d1aca

…_8174_so

Merge branch 'test_issue_8174_so' of https://github.com/Kai-Striega/s…

ffec160

…cipy into test_issue_8174_so

mdhaber reviewed Aug 3, 2018

View reviewed changes

Fix test logic to remove redundant condition..

934b255

mdhaber reviewed Aug 4, 2018

View reviewed changes

Fix test logic.

0030cc1

Kai-Striega force-pushed the test_issue_8174_so branch from fc2d7c3 to 0030cc1 Compare August 4, 2018 04:53

Remove duplicate test to assert success.

ba6566d

ilayn added this to the 1.2.0 milestone Aug 9, 2018

mdhaber merged commit 1a90a22 into scipy:master Aug 9, 2018

rgommers added the maintenance Items related to regular maintenance tasks label Aug 9, 2018

Kai-Striega deleted the test_issue_8174_so branch August 15, 2018 02:08

mdhaber mentioned this pull request Sep 14, 2018

ENH: linprog in SciPy 1.2+ discussion #9269

Closed

22 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Add test for Stackoverflow example from issue 8174. #9096

BUG: Add test for Stackoverflow example from issue 8174. #9096

Kai-Striega commented Jul 31, 2018

mdhaber commented Jul 31, 2018

Kai-Striega commented Jul 31, 2018

Kai-Striega commented Aug 2, 2018

mdhaber left a comment

mdhaber Aug 2, 2018

mdhaber Aug 2, 2018

Kai-Striega Aug 3, 2018

mdhaber Aug 3, 2018 •

edited

Loading

mdhaber Aug 3, 2018 •

edited

Loading

Kai-Striega Aug 3, 2018 •

edited

Loading

Kai-Striega Aug 3, 2018

mdhaber commented Aug 2, 2018 •

edited

Loading

mdhaber Aug 3, 2018

Kai-Striega Aug 3, 2018

mdhaber Aug 3, 2018

mdhaber Aug 3, 2018

Kai-Striega Aug 3, 2018

mdhaber Aug 3, 2018 •

edited

Loading

Kai-Striega Aug 3, 2018

mdhaber Aug 4, 2018 •

edited

Loading

mdhaber Aug 4, 2018

mdhaber Aug 4, 2018

Kai-Striega commented Aug 4, 2018

mdhaber commented Aug 4, 2018 •

edited

Loading

Kai-Striega commented Aug 4, 2018 •

edited

Loading

Kai-Striega commented Aug 9, 2018

mdhaber commented Aug 9, 2018

rgommers commented Aug 9, 2018

BUG: Add test for Stackoverflow example from issue 8174. #9096

BUG: Add test for Stackoverflow example from issue 8174. #9096

Conversation

Kai-Striega commented Jul 31, 2018

mdhaber commented Jul 31, 2018

Kai-Striega commented Jul 31, 2018

Kai-Striega commented Aug 2, 2018

mdhaber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdhaber Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

mdhaber Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

Kai-Striega Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdhaber commented Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdhaber Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdhaber Aug 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kai-Striega commented Aug 4, 2018

mdhaber commented Aug 4, 2018 • edited Loading

Kai-Striega commented Aug 4, 2018 • edited Loading

Kai-Striega commented Aug 9, 2018

mdhaber commented Aug 9, 2018

rgommers commented Aug 9, 2018

mdhaber Aug 3, 2018 •

edited

Loading

mdhaber Aug 3, 2018 •

edited

Loading

Kai-Striega Aug 3, 2018 •

edited

Loading

mdhaber commented Aug 2, 2018 •

edited

Loading

mdhaber Aug 3, 2018 •

edited

Loading

mdhaber Aug 4, 2018 •

edited

Loading

mdhaber commented Aug 4, 2018 •

edited

Loading

Kai-Striega commented Aug 4, 2018 •

edited

Loading