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: 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 mkoeppe)

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 mkoeppe

  • Branch set to u/mkoeppe/add_copy___copy___methods_to_cvxopt__ppl__interactivelp_backends

comment:2 Changed 3 years ago by git

  • Commit set to cdd159d165c33eff92e006f435a3a0c7384a2487

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

0bd7a87GenericBackend._test_add_variables(): Add tests from CVXOPTBackend
6b55e16Add a classmethod _test_solve
e14afd3GenericBackend._test_solve: Finish
48b9fe5Change from mutating instance _test methods to class methods
5394729New method _test_ncols_nonnegative
7138fa0GenericBackend: Remove failing _test methods from this ticket to make the patchbot and its friends happy
452b406Merge branch 'u/mkoeppe/common_testsuite_for_mip_backends' into t/20414/add_copy___copy___methods_to_cvxopt__ppl__interactivelp_backends
8f968ed_test_copy: New
3e26c4d_test_copy_does_not_share_data: New
cdd159dtest_copy_some_mips: New

comment:3 Changed 3 years ago by git

  • Commit changed from cdd159d165c33eff92e006f435a3a0c7384a2487 to 4b9fd051aa512b59351de115b21c744d9c312e8e

Branch pushed to git repo; I updated commit sha1. New commits:

4b9fd05InteractiveLPBackend.__copy__: New

comment:4 Changed 3 years ago by git

  • Commit changed from 4b9fd051aa512b59351de115b21c744d9c312e8e to 580e913062ab40b595fdb508d94b06c44a384f40

Branch pushed to git repo; I updated commit sha1. New commits:

fe1a8b9PPLBackend: Implement __copy__
d9f1ec8CVXOPTBackend: Implement __copy__
751d621InteractiveLPBackend.__copy__: Fix doctest
9d39e0cCoinBackend: In add_col, don't forget to append to col_names
7c97097GenericBackend.__copy__: Fix doctest
ce10c70GenericBackend._test_copy_some_mips: Change test so it does not use 0 coefficients
580e913GenericBackend._do_test_problem_data: Compare types as well

comment:5 Changed 3 years ago by mkoeppe

  • 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 git

  • Commit changed from 580e913062ab40b595fdb508d94b06c44a384f40 to 38c6be9df03439d604f7b6c476cbebd6e606e7fd

Branch pushed to git repo; I updated commit sha1. New commits:

38c6be9MixedIntegerLinearProgram, GenericBackend: Add __deepcopy__ methods

comment:7 Changed 3 years ago by mkoeppe

  • Description modified (diff)

Now also contains a fix for #15159 (see description). Needs review.

comment:8 Changed 3 years ago by mkoeppe

  • 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 mkoeppe

  • Authors set to Matthias Koeppe

comment:10 follow-up: Changed 3 years ago by dimpase

How reliable is __deepcopy__ here? Is it just working now?

comment:11 Changed 3 years ago by git

  • Commit changed from 38c6be9df03439d604f7b6c476cbebd6e606e7fd to 3117b01e595febc8fc6de90eeb3cf19a7e97c7bd

Branch pushed to git repo; I updated commit sha1. New commits:

3117b01MixedIntegerLinearProgram.__deepcopy__: Add another doctest

comment:12 in reply to: ↑ 10 ; follow-up: Changed 3 years ago by mkoeppe

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

Replying to mkoeppe:

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.

was there something about variables that are (not) copied?

comment:14 in reply to: ↑ 13 Changed 3 years ago by mkoeppe

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

  • 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 mkoeppe

Replying to dimpase:

ok, fine.

Thanks for reviewing.

I've created a new ticket, #20461, for solving the issue with the variables of a copied MIP. Please take a look when you have a chance.

comment:17 Changed 3 years ago by vbraun

  • 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
Note: See TracTickets for help on using tickets.