MAINT: Add more descriptive error message for phase one simplex. #8958
Conversation
Phase one of the simplex method sometimes fails to converge to a basic feasible solution if the pseudo-objective function is very slightly greater than the set tolerance. The new message states this, the tolerance and pseudo-objective function value explicitly
scipy/optimize/_linprog.py
Outdated
| @@ -789,9 +789,21 @@ def _linprog_simplex(c, A_ub=None, b_ub=None, A_eq=None, b_eq=None, | |||
| else: | |||
| # Failure to find a feasible starting point | |||
| status = 2 | |||
| message = ( | |||
There was a problem hiding this comment.
This message is much better than what I wrote!
Only suggestion - and this is nitpicky and subjective, so you decide for yourself - is to set messages[status] to this string rather than setting message directly. Then you can get rid of the line you had to add if not message and that extra level of indentation that goes with it. Finally, since it would be a single line change, it's easier to see that it won't break anything else. Let me know if you'd like to leave as is or change it; I'll get a second opinion for good measure and then get this merged.
|
That's a nicer way to add it @mdhaber. Given how problematic this has been I've also added a quick test to make sure changing changing the tolerance will return a correct result. |
|
Tests are a good idea. I thought about suggesting it but I know the tolerance might need to be different on different machines. But hopefully your 1e-9 should work all the time; I guess we'll see. @ev-br I see you looked at #6139 in the past. Turns out it wasn't really a bug, just a default tolerance issue. What do you think of this fix - just a more descriptive error message? @ |
|
Regarding the tests I had initially considered setting the |
rgommers
added
scipy.optimize
maintenance
Linprog(method='simplex') fails to find a basic feasible solution if phase 1 pseudo-objective function is outside the provided tol. Suppress `OptimizeWarning` for program presolve in 7044.
|
@mdhaber would you be able to have a look at this? There have been some tests failing due to tests which were not run when #6139 was merged relating to warnings. In addition to the tests for the new error message, I've added warning supressions from the inputs. Is there some way to ensure that all tests are run so this kind of thing is not repeated? Edit: It seems like this is a problem for more than just my work. There are still some failing tests due other missed warnings. They have been fixed in #8975 by @larsoner. Would you have any idea why these were only run after the merge? |
|
I've been following this. It sounds like it's a coincidence; some change was made after the merge that made these warnings cause failures. I think you've done all we can do - add warning suppressions where appropriate. Looks like this is all set? |
|
All good with me. |
scipy/optimize/tests/test_linprog.py
Outdated
| low_tol = 1e-20 | ||
| res = linprog( | ||
| c, A_ub, b_ub, A_eq, b_eq, | ||
| bounds=bounds, options={'tol': low_tol} |
There was a problem hiding this comment.
Here and below I would explicitly set method='self.method' in case the default algorithm is changed later. Sorry I didn't catch it before.
|
Does anything else stand out, or is this ready to be added? |
|
Another one down! |
|
Really good to see |
|
Thanks for the compliment! Do you think it's time to close #6139? I don't see anything else we can do about it. |
|
Thanks for the reminder. |
Phase one of the simplex method sometimes fails to converge to a
basic feasible solution if the pseudo-objective function is very
slightly greater than the set tolerance. The new message states
this, the tolerance and pseudo-objective function value explicitly