Opened 4 years ago

Closed 4 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) 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 4 years ago by mkoeppe

  • Dependencies changed from #20351 to #20351,#20303

comment:2 Changed 4 years ago by mkoeppe

  • Branch set to u/mkoeppe/pplbackend__add_support_for_integer_variables

comment:3 Changed 4 years ago by mkoeppe

  • Commit set to 144a970ae97b209acccf6d38de540401c8c8a587
  • Status changed from new to needs_review

Currently this branch is on top of the branches for #20351,#20303.

Needs review.


Last 10 new commits:

9f35b65Wrap MIP_Problem.add_to_integer_space_dimensions
39473c9Fix and FIXME
bceca77CVXOPTBackend: Use 'is None' instead of '== None'
728750fCVXOPTBackend.add_variables: Pass arguments to add_variable, correct default for lower_bound
b4a8ed7Copy new CVXOPTBackend.add_variables tests to other backends
b0d89e4GLPKBackend.add_variables: Set column names correctly
183ce25PPLBackend.add_variable, add_variables: Don't silently ignore binary=True, integer=True
e47b608CVXOPTBackend.add_linear_constraints: Add doctest, simplify code
b0179c2Merge branch 't/20303/fixes_for_the_cvxopt_mip_backend' into t/20354/pplbackend__add_support_for_integer_variables
144a970PPLBackend: Add support for integer variables

comment:4 Changed 4 years ago by mkoeppe

  • Authors set to Matthias Koeppe

comment:5 follow-up: Changed 4 years ago by 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?

comment:6 in reply to: ↑ 5 ; follow-up: Changed 4 years ago by 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. 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 4 years ago by dimpase

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 4 years ago by mkoeppe

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: Changed 4 years ago by dimpase

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 4 years ago by mkoeppe

Replying to dimpase:

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.

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: Changed 4 years ago by 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, let set_variable_type accept the same range of values, and deprecate 0,1,-1 ?

comment:12 in reply to: ↑ 11 Changed 4 years ago by mkoeppe

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, let set_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 4 years ago by dimpase

  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to positive_review

OK, looks good.

comment:14 Changed 4 years ago by vbraun

  • Branch changed from u/mkoeppe/pplbackend__add_support_for_integer_variables to 144a970ae97b209acccf6d38de540401c8c8a587
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.