Opened 4 years ago
Closed 4 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: | sage-7.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 4 years ago by
- Branch set to u/mkoeppe/add_copy___copy___methods_to_cvxopt__ppl__interactivelp_backends
comment:2 Changed 4 years ago by
- Commit set to cdd159d165c33eff92e006f435a3a0c7384a2487
comment:3 Changed 4 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 4 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 4 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 4 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 4 years ago by
- Description modified (diff)
Now also contains a fix for #15159 (see description). Needs review.
comment:8 Changed 4 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 4 years ago by
comment:10 follow-up: ↓ 12 Changed 4 years ago by
How reliable is __deepcopy__
here? Is it just working now?
comment:11 Changed 4 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 ; follow-up: ↓ 13 Changed 4 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 ; follow-up: ↓ 14 Changed 4 years ago by
comment:14 in reply to: ↑ 13 Changed 4 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 follow-up: ↓ 16 Changed 4 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 4 years ago by
comment:17 Changed 4 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