Opened 3 years ago
Closed 3 years ago
#20414 closed defect (fixed)
Add copy/__copy__ methods to CVXOPT, PPL, InteractiveLP backends, and __deepcopy__ to MixedIntegerLinearProgram and backends
Reported by:  mkoeppe  Owned by:  

Priority:  major  Milestone:  sage7.2 
Component:  numerical  Keywords:  lp 
Cc:  vdelecroix, vbraun, dimpase  Merged in:  
Authors:  Matthias Koeppe  Reviewers:  Dima Pasechnik 
Report Upstream:  N/A  Work issues:  
Branch:  3117b01 (Commits)  Commit:  3117b01e595febc8fc6de90eeb3cf19a7e97c7bd 
Dependencies:  Stopgaps: 
Description (last modified by )
The COIN, CPLEX, GLPK, and Gurobi backends supply a copy
method.
But it's not documented as a backend interface method in GenericBackend
, and the CVXOPT, PPL, InteractiveLP backends do not implement it.
It is used by MixedIntegerLinearProgram.__copy__
.
The backend method should actually probably be called __copy__
as well  see https://docs.python.org/2/library/copy.html
The branch on the ticket does this.
We also add __deepcopy__
methods to MixedIntegerLinearProgram
and the backends.
This fixes #15159, though the semantics of copy
and deepcopy
of a MixedIntegerLinearProgram
remains questionable due to its interaction with MIPVariable
; see #15159 and #19523.
See also #20323 (in which copying a backend could be the basis for more diverse tests).
Change History (17)
comment:1 Changed 3 years ago by
 Branch set to u/mkoeppe/add_copy___copy___methods_to_cvxopt__ppl__interactivelp_backends
comment:2 Changed 3 years ago by
 Commit set to cdd159d165c33eff92e006f435a3a0c7384a2487
comment:3 Changed 3 years ago by
 Commit changed from cdd159d165c33eff92e006f435a3a0c7384a2487 to 4b9fd051aa512b59351de115b21c744d9c312e8e
Branch pushed to git repo; I updated commit sha1. New commits:
4b9fd05  InteractiveLPBackend.__copy__: New

comment:4 Changed 3 years ago by
 Commit changed from 4b9fd051aa512b59351de115b21c744d9c312e8e to 580e913062ab40b595fdb508d94b06c44a384f40
Branch pushed to git repo; I updated commit sha1. New commits:
fe1a8b9  PPLBackend: Implement __copy__

d9f1ec8  CVXOPTBackend: Implement __copy__

751d621  InteractiveLPBackend.__copy__: Fix doctest

9d39e0c  CoinBackend: In add_col, don't forget to append to col_names

7c97097  GenericBackend.__copy__: Fix doctest

ce10c70  GenericBackend._test_copy_some_mips: Change test so it does not use 0 coefficients

580e913  GenericBackend._do_test_problem_data: Compare types as well

comment:5 Changed 3 years ago by
 Status changed from new to needs_review
The branch is on top of #20323 (Common TestSuite
for MIP backends).
The _test_copy_some_mips
method exposed a bug in CoinBackend
, which I have fixed on the way.
comment:6 Changed 3 years ago by
 Commit changed from 580e913062ab40b595fdb508d94b06c44a384f40 to 38c6be9df03439d604f7b6c476cbebd6e606e7fd
Branch pushed to git repo; I updated commit sha1. New commits:
38c6be9  MixedIntegerLinearProgram, GenericBackend: Add __deepcopy__ methods

comment:7 Changed 3 years ago by
 Description modified (diff)
Now also contains a fix for #15159 (see description). Needs review.
comment:8 Changed 3 years ago by
 Summary changed from Add copy/__copy__ methods to CVXOPT, PPL, InteractiveLP backends to Add copy/__copy__ methods to CVXOPT, PPL, InteractiveLP backends, and __deepcopy__ to MixedIntegerLinearProgram and backends
comment:9 Changed 3 years ago by
comment:10 followup: ↓ 12 Changed 3 years ago by
How reliable is __deepcopy__
here? Is it just working now?
comment:11 Changed 3 years ago by
 Commit changed from 38c6be9df03439d604f7b6c476cbebd6e606e7fd to 3117b01e595febc8fc6de90eeb3cf19a7e97c7bd
Branch pushed to git repo; I updated commit sha1. New commits:
3117b01  MixedIntegerLinearProgram.__deepcopy__: Add another doctest

comment:12 in reply to: ↑ 10 ; followup: ↓ 13 Changed 3 years ago by
Replying to dimpase:
How reliable is
__deepcopy__
here? Is it just working now?
It's working. I've added another test to illustrate deepcopy semantics.
comment:13 in reply to: ↑ 12 ; followup: ↓ 14 Changed 3 years ago by
comment:14 in reply to: ↑ 13 Changed 3 years ago by
Replying to dimpase:
was there something about variables that are (not) copied?
Before this patch, deepcopy didn't even copy the backend of a MIP. Now it does, simply by doing the same as copy.
Both copy and deepcopy have weird semantics in relation to the MIPVariables in a problems  because a MIP does not have an API to gain access to its variables; and trying to use the old MIPVariables with the copy is questionable and definitely broken if one creates new components of this variable, see #15159, #19523. Fixing this would require a redesign; a possible stopgap would be to stop all MIPVariables from creating new components when they have been subjected to copy().
comment:15 followup: ↓ 16 Changed 3 years ago by
 Reviewers set to Dima Pasechnik
 Status changed from needs_review to positive_review
ok, fine.
comment:16 in reply to: ↑ 15 Changed 3 years ago by
comment:17 Changed 3 years ago by
 Branch changed from u/mkoeppe/add_copy___copy___methods_to_cvxopt__ppl__interactivelp_backends to 3117b01e595febc8fc6de90eeb3cf19a7e97c7bd
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
GenericBackend._test_add_variables(): Add tests from CVXOPTBackend
Add a classmethod _test_solve
GenericBackend._test_solve: Finish
Change from mutating instance _test methods to class methods
New method _test_ncols_nonnegative
GenericBackend: Remove failing _test methods from this ticket to make the patchbot and its friends happy
Merge branch 'u/mkoeppe/common_testsuite_for_mip_backends' into t/20414/add_copy___copy___methods_to_cvxopt__ppl__interactivelp_backends
_test_copy: New
_test_copy_does_not_share_data: New
test_copy_some_mips: New