Opened 5 years ago
Closed 5 years ago
#20354 closed enhancement (fixed)
PPLBackend: Add support for integer variables
Reported by: | mkoeppe | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-7.2 |
Component: | numerical | Keywords: | lp |
Cc: | dimpase, vdelecroix, jdemeyer | Merged in: | |
Authors: | Matthias Koeppe | Reviewers: | Dima Pasechnik |
Report Upstream: | N/A | Work issues: | |
Branch: | 144a970 (Commits, GitHub, GitLab) | Commit: | 144a970ae97b209acccf6d38de540401c8c8a587 |
Dependencies: | #20351,#20303 | Stopgaps: |
Description
PPL's solver is a rational *MIP* solver. Its support for integer variables should be exposed in Sage. #20351 does this for the PPL wrapper classes.
This ticket updates the PPLBackend
class.
Change History (14)
comment:1 Changed 5 years ago by
- Dependencies changed from #20351 to #20351,#20303
comment:2 Changed 5 years ago by
- Branch set to u/mkoeppe/pplbackend__add_support_for_integer_variables
comment:3 Changed 5 years ago by
- Commit set to 144a970ae97b209acccf6d38de540401c8c8a587
- Status changed from new to needs_review
comment:4 Changed 5 years ago by
comment:5 follow-up: ↓ 6 Changed 5 years ago by
hmm:
cpdef int add_variable(self, lower_bound=0, upper_bound=None, binary=False, continuous=False, integer=False, obj=0, name=None) except -1:
should we really pick a default (continuous
seems to be the most natural) rather than being it total denial?
comment:6 in reply to: ↑ 5 ; follow-up: ↓ 7 Changed 5 years ago by
Replying to dimpase:
hmm:
cpdef int add_variable(self, lower_bound=0, upper_bound=None, binary=False, continuous=False, integer=False, obj=0, name=None) except -1:should we really pick a default (
continuous
seems to be the most natural) rather than being it total denial?
This idiosyncratic interface (including the documentation that says that the default value of 'continuous' is True) matches that of the other backends.
I would probably express this by making all arglist-default None
to indicate complicated defaulting semantics. But it's not the job of this ticket to make changes throughout all backends.
comment:7 in reply to: ↑ 6 Changed 5 years ago by
Replying to mkoeppe:
Replying to dimpase:
hmm:
cpdef int add_variable(self, lower_bound=0, upper_bound=None, binary=False, continuous=False, integer=False, obj=0, name=None) except -1:should we really pick a default (
continuous
seems to be the most natural) rather than being it total denial?This idiosyncratic interface (including the documentation that says that the default value of 'continuous' is True) matches that of the other backends.
but this is a line from your update, why do you set continuous=False
?
Something you changed while developing and then forgot to revert?
comment:8 Changed 5 years ago by
No, continuous=True
in the arglist cannot work
because then one cannot distinguish
add_variable(..., binary=True)
(which should create a binary variable)add_variable(..., binary=True, continuous=True)
(which should raise an error).
All backends implement the defaulting to continuous variables imperatively.
Previously it did not matter because the PPL backend was simply ignoring these parameters altogether.
(I've added a dicussion of this interface to ticket #20324.)
comment:9 follow-up: ↓ 10 Changed 5 years ago by
OK, I see. I think that the design with these logical switches is just wrong. There should be variable_type
parameter, defaulting to continuous
, with other possible values binary
and integer
.
comment:10 in reply to: ↑ 9 Changed 5 years ago by
Replying to dimpase:
I think that the design with these logical switches is just wrong. There should be
variable_type
parameter, defaulting tocontinuous
, with other possible valuesbinary
andinteger
.
I agree; at least it matches is_variable_continuous
etc.; but see set_variable_type
, which expresses the same thing using 1, 0, -1.
It's one of several idiosyncrasies of the backend interface.
comment:11 follow-up: ↓ 12 Changed 5 years ago by
How about introducing the variable_type
parameter and deprecating the rest? (This is probably quite a bit of things to be changed then, in graphs/combinatorics code)
Similarly, let set_variable_type
accept the same range of values, and deprecate 0,1,-1 ?
comment:12 in reply to: ↑ 11 Changed 5 years ago by
Replying to dimpase:
How about introducing the
variable_type
parameter and deprecating the rest? (This is probably quite a bit of things to be changed then, in graphs/combinatorics code) Similarly, letset_variable_type
accept the same range of values, and deprecate 0,1,-1 ?
Sounds good; I have created ticket #20362 for this. Let's move further discussion of backend interface redesign there.
In the meantime, this ticket still needs review.
comment:13 Changed 5 years ago by
- Reviewers set to Dima Pasechnik
- Status changed from needs_review to positive_review
OK, looks good.
comment:14 Changed 5 years ago by
- Branch changed from u/mkoeppe/pplbackend__add_support_for_integer_variables to 144a970ae97b209acccf6d38de540401c8c8a587
- Resolution set to fixed
- Status changed from positive_review to closed
Currently this branch is on top of the branches for #20351,#20303.
Needs review.
Last 10 new commits:
Wrap MIP_Problem.add_to_integer_space_dimensions
Fix and FIXME
CVXOPTBackend: Use 'is None' instead of '== None'
CVXOPTBackend.add_variables: Pass arguments to add_variable, correct default for lower_bound
Copy new CVXOPTBackend.add_variables tests to other backends
GLPKBackend.add_variables: Set column names correctly
PPLBackend.add_variable, add_variables: Don't silently ignore binary=True, integer=True
CVXOPTBackend.add_linear_constraints: Add doctest, simplify code
Merge branch 't/20303/fixes_for_the_cvxopt_mip_backend' into t/20354/pplbackend__add_support_for_integer_variables
PPLBackend: Add support for integer variables