-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
MAINT: Refactor linear programming solvers #9145
Conversation
Move ``_clean_inputs(...)``, ``_presolve(...)``, ``_get_Abc(...)`` and ``_post_process(...)`` from module ``_linprog_ip`` into ``_linprog``. Remove duplicate behavior from ``_linprog_simplex(...)`` and modify return values to match those required in pre and postsolve functions.
83f328e
to
4949184
Compare
Excited about this! I'll try to look today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! Tough to follow because of how GitHub detects additions/deletions but I think it all makes sense and doesn't look too dangerous. I wrote a bunch of comments about what I think was changed and why. These are just to confirm that I checked the changed and tried to make sense of them, but if any are wrong, would you let me know?
I didn't check the test changes very carefully. Anything substantial, besides relaxing some of the tests that previously were not applied to simplex?
Only thing I saw that I was a little concerned about was pop
ing options. I don't understand why get
isn't sufficient.
scipy/optimize/_linprog.py
Outdated
tol = options.get('tol', default_tol) | ||
|
||
# This is an undocumented option for unit testing sparse presolve | ||
_sparse_presolve = options.pop('_sparse_presolve', False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why pop
it (instead of get
it)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_sparse_presolve
applies only to this part of the program (changing A to sparse for testing). Previously this was done in _linprog_ip(...)
with _sparse_presolve
passed as a (undocumented) option. The conversion now occurs in _linprog(...)
. _linprog_ip(...)
no longer requires _sparse_presolve
to be passed as an option, and has been removed. If options
still contains _sparse_presolve
it will be passed to _check_unknown_options(...)
, not be recognized as an option (as it no longer is) and raise an error.
scipy/optimize/_linprog.py
Outdated
|
||
# Solve trivial problem, eliminate variables, tighten bounds, etc... | ||
c0 = 0 # we might get a constant term in the objective | ||
if options.pop('presolve', True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again why are we modifying options
? This is a reference to the dictionary passed in by the user, not our own local copy, right? So this would modify the user's object, which we wouldn't want to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same reasoning as for _sparse_presolve
.
I considered making a local copy of options
, however the user's dictionary would not be used anywhere else. Unless you think keeping it as a reference is worth doing I don't see a benefit to keeping a duplicate copy.
scipy/optimize/_linprog.py
Outdated
|
||
# Solve trivial problem, eliminate variables, tighten bounds, etc... | ||
c0 = 0 # we might get a constant term in the objective | ||
if options.pop('presolve', True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again why are we modifying options
here and anywhere else rather than just reading it? This is a reference to the dictionary passed in by the user, not our own local copy, right? So this would modify the user's object, which we wouldn't want to do.
@@ -1754,11 +759,9 @@ def eta(g=gamma): | |||
|
|||
def _linprog_ip( | |||
c, | |||
A_ub=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These deletions make sense because the problem is already in standard form.
A_eq=None, | ||
b_eq=None, | ||
bounds=None, | ||
c0=0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense because presolve might have added constant to objective - which is just needed in here for display, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, c0
will only be used for display.
@@ -0,0 +1,496 @@ | |||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_linprog_simplex
, _solve_simplex
, _apply_pivot
, _pivot_row
, and _pivot_col
simply moved from _linprog.py
scipy/optimize/tests/test_linprog.py
Outdated
sup.filter(OptimizeWarning, "Solving system with option...") | ||
res = linprog(c=cost, A_eq=A_eq, b_eq=b_eq, bounds=bounds, | ||
method=self.method, options=self.options) | ||
method=self.method, options=self.options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the indentation of this line is off? Should match with ( on previous line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll fix it in the next commit
scipy/optimize/tests/test_linprog.py
Outdated
@@ -874,7 +885,7 @@ def cb(xk, **kwargs): | |||
res = linprog(c, A_ub=A_ub, b_ub=b_ub, callback=cb, method=self.method) | |||
|
|||
assert_(callback_complete[0]) | |||
assert_allclose(last_xk[0], res.x) | |||
assert_allclose(last_xk[0][:res.x.size], res.x) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was there a bug in this test before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The simplex method sees the artificial variables / variable substitutions from pre processing as "true" variables expecting them to appear in the solution. The variables are instead removed during _postprocess
and therefore do not appear.
res = linprog( | ||
c, A_ub, b_ub, A_eq, b_eq, method=self.method, | ||
bounds=bounds, options={'tol': low_tol} | ||
bounds=bounds, options=self.options |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not digging into this - can you explain why this changed?
if self.method == "simplex": | ||
# Including the callback here ensures the solution can be | ||
# calculated correctly. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool that several of these tests pass with simple now.
CI failure is due to a doctest in AppVeyor finding some assertion errors in simplex. Not sure what's going on there. |
Maybe also check whether |
I think the number of iterations are different because of using The relevant output from AppVoyeur:
on my system:
So it shouldn't really be happening. Maybe it has to do with only being on python 2.7? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried to make it at least halfway. I would really recommend checking out previous code to stay coherent with the remaining parts of the library.
And mostly for the logical comparisons we can be a bit more concise.
scipy/optimize/_linprog.py
Outdated
from warnings import warn | ||
from .optimize import OptimizeResult, OptimizeWarning, _check_unknown_options | ||
from scipy.optimize._remove_redundancy import _remove_redundancy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to group these imports with (
on the first one and )
on the last over multiple lines.
scipy/optimize/_linprog.py
Outdated
regardless of magnitude). | ||
c : array_like | ||
Coefficients of the linear objective function to be minimized. | ||
A_ub : array_like, optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can simply use the [...] such that A_ub @ x gives the upper bound constraints
or something similar. Also -
is not required for dimension notation to be consistent with the rest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was simply copied from _linporg_ip
. The description has NOT been consistent between additions. Standardising the descriptions will need to be done at some point, preferably before anything new is added.
scipy/optimize/_linprog.py
Outdated
1-D array of values representing the RHS of each equality constraint | ||
(row) in ``A_eq``. | ||
bounds : sequence, optional | ||
``(min, max)`` pairs for each element in ``x``, defining |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it have to be tuple? list of tuples work? Also the explanation might use some explicit example :
[...] [(a1, b1), (a2, b2) ...]
defining upper bounds on the parameter. By default all a
s are set to 0 and all b
s to None hence modeling the interval [0, inf]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree these SHOULD be a different form. This is how they were implemented as standard @mdhaber and myself have already noted that it should be changed, but in a separate PR.
scipy/optimize/_linprog.py
Outdated
|
||
c : 1-D array | ||
Coefficients of the linear objective function to be minimized. | ||
A_ub : 2-D array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above with dimension dashes and A_ub@x
. Why do you explain the outputs if they are going to be checked anyways.
scipy/optimize/_linprog.py
Outdated
bland : bool | ||
If True, use Bland's rule for selection of the row (if more than one | ||
row can be used, choose the one with the lowest variable index). | ||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty nonstandard array checking. You can already use the misc.utils functions for checking valid input arraylikes. You don't want to chase exception types but trust on np.arrayability. Those functions will complain anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't know about those when I wrote it. It's quite thorough and works very well, so I hope we don't have to overhaul this right now. See history of such comments in #7123.
scipy/optimize/_linprog.py
Outdated
ub_newub = ubs[ub_some] | ||
n_bounds = np.count_nonzero(ub_some) | ||
A_ub = vstack((A_ub, zeros((n_bounds, A_ub.shape[1])))) | ||
b_ub = np.concatenate((b_ub, np.zeros(n_bounds))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hstack maybe ? Since you have vstack elsewhere?
scipy/optimize/_linprog.py
Outdated
|
||
# add slack variables | ||
A2 = vstack([eye(A_ub.shape[0]), zeros((A_eq.shape[0], A_ub.shape[0]))]) | ||
A = hstack([A1, A2]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah you have hstack afterall
scipy/optimize/_linprog.py
Outdated
where: -inf <= x[0] <= inf | ||
Parameters | ||
---------- | ||
x : 1-D array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comments as above.
scipy/optimize/_linprog.py
Outdated
1 : Iteration limit reached | ||
2 : Problem appears to be infeasible | ||
3 : Problem appears to be unbounded | ||
4 : Serious numerical difficulties encountered |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is not "serious" in terms of difficulties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Difficulties that have been resolved using a more robust, albeit less efficient, solver.
scipy/optimize/_linprog.py
Outdated
optimal objective value for original problem | ||
slack: 1-D array | ||
The (non-negative) slack in the upper bound constraints, that is, | ||
``b_ub - A_ub * x`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A_ub @ x ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You know I just learned about that a few weeks ago?
@ilayn Thanks for all these checks but can we wait for a separate PR to fix existing issues in the code? Most of the stuff that shows up in green is not new; it is just moved from elsewhere. |
@ilayn Thanks for having such a thorough look through it! I'm going to respond to most of the reviews within a single comment. I have not written most of the code. The PR moves existing code from |
Ah I was going through only the green lines and didn't pay too much attention to the existing code. In that case indeed let's keep it as only the refactoring. But we at least need the |
Based on the discussion with @mdhaber and @ilayn, here is what I think we should be looking at for merging the solvers: Modifying
|
@Kai-Striega more to come, but why don't we just change it so that the options are not unknown? We should also add |
Also the thoughts about docstrings sound great. |
Is there a way to add |
@Kai-Striega It doesn't bother me. If it really bothers you, you could copy the dictionary.
Or perhaps someone else has a more clever solution. But I think it's more trouble than it's worth. |
Changes to the ``simplex`` method now provide a more robust simplex solver. Tests previously only passing for ``interior-point`` now also pass for ``simplex``. The tests from ``BaseTestLinprogIP`` have been moved into ``LinprogCommonTests`` to reflect this.
``test_issue_6139`` incorrectly failed for ``TestLinprogIPDense`` due to the tolerance being set aribtrarily low. The commit increases the ``tol`` value inside ``_postprocess(...)`` by a factor of 10.
By default linprog attempts to presolve a problem. If the problem is solved linprog avoids using a solver at all. However the test expects a NotImplementedError to be raised regardless of if a callback is actually required. The test now expects a NotImplementedError only if a callback would actually have been used.
Hey Kai, this is pretty close? Let me know when you'd like me to take another look. |
@mdhaber I'm getting close. I've been busy with uni giving me less time to work on this. The doc string for I still haven't changed how Ideally I'll have time to get the above working today or tomorrow. If you would like to have a look at it those changes will be independent of the changes so far. |
that sounds good to me. |
@mdhaber @rgommers In summary, the signature for EDIT: Adding only an |
- Add ``# may vary`` to output slack arguments - Add unit test for docstring example - Extend docstring for status 4 to be more descriptive
…into merge_linprog
@Kai-Striega Please review the comments. As you can see, we were reluctant to ask for this change, as we know this PR was intended only to refactor code. However, the PR as it stood broke backwards compatibility silently by changing the meaning of the information passed to the callback function without changing the signature. Unfortunatley, this was considered unacceptable. We iterated on how the situation should be handled, and decided that the best course of action - easiest for now and sufficiently flexible for the future - would be to pass an You are welcome to modify Thanks! |
651e4f4
to
42c9779
Compare
Recent changes to ``linprog`` silently break the (undefined) behaviour of ``callback``. The commit changes the callback signature to require an ``OptimizeResult`` rather than the existing ``xk``, ``kwargs`` by refactoring ``_postprocess`` into multiple, smaller, functions in a new module ``_linprog_util``.
42c9779
to
246dc22
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kai-Striega Looks great! I don't see very much I'd change. I really like the callback test's way of verifying that the callback function is receiving the information it is supposed to. It is good that problem chosen for the callback test has inequality constraints because then it is known to have variables added in conversion to standard form, so checking, for instance, that the values of x
inside the callback are equal to those reported at completion is meaningful.
I might have checked all the fields of the OptimizeResult
and chosen a problem that would also have been modified in presolve, and I think that the name of _T_o
/T_o
is descriptive enough.
The only thing I have to ask for is that the documentation of the callback function argument for linprog
and linprog(method='simplex')
be modified. They still specify the old signature and reference the tableau.
@rgommers See anything else that needs to be changed, or can this be merged once the documentation is updated? Regarding the documentation, can you suggest how to call attention to the compatibility break (in 1.2, presumably) and the rationale behind it? |
Notices of backward-compatibility issues should be added to the release notes. |
Draft release notes edited to reflect this change. @Kai-Striega You're welcome to edit if they could be more clear; just wanted to help. |
@mdhaber I've edited the docstrings as requested, plus added more tests for I've done some further refactoring by moving the private functions from Setting the |
@Kai-Striega Took a quick look; it appears that you've updated the docstrings and moved things around in a way that makes sense. Looking forward to the checks passing! If so, are you done? |
Yes. This should be it. I think the PR is getting too unwieldly anyway so any extra changes should really be in a new PR |
666363f
to
c99aff9
Compare
@rgommers Think this is ready? Do the draft release notes adequately describe the change? |
Release note update looks good to me. Go ahead and merge if code is good to go. |
Early implementation of changes discussed in #8942 and #9076. It's far from complete but I think it's worth discussing the changes so far.
Major changes:
_linrpog.py
_linrpog_simplex(…)
.The effects on the current issues:
For 5400 and 8174 the simplex method pivots with a tolerance value very close to zero. The solution then becomes (wrongly) infeasible with a status of 0 (success). A warning is raised during the simplex method if this occurred. An error should be raised instead. Post-process catches this but returns a status of 4 and an error message instead of an error, should this be changed to a ValueError?
There are more less important changes, most of which are added as comments:
@mdhaber again this is far from complete, but how does this look so far?