Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add copy/__copy__ methods to CVXOPT, PPL, InteractiveLP backends, and __deepcopy__ to MixedIntegerLinearProgram and backends #20414

Closed
mkoeppe opened this issue Apr 11, 2016 · 23 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 11, 2016

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).

CC: @videlec @vbraun @dimpase

Component: numerical

Author: Matthias Koeppe

Branch/Commit: 3117b01

Reviewer: Dima Pasechnik

Issue created by migration from https://trac.sagemath.org/ticket/20414

@mkoeppe mkoeppe added this to the sage-7.2 milestone Apr 11, 2016
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 14, 2016

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 15, 2016

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 15, 2016

Commit: cdd159d

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 15, 2016

Changed commit from cdd159d to 4b9fd05

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 15, 2016

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

4b9fd05InteractiveLPBackend.__copy__: New

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 15, 2016

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 15, 2016

Changed commit from 4b9fd05 to 580e913

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 15, 2016

comment:5

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 17, 2016

Changed commit from 580e913 to 38c6be9

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 17, 2016

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

38c6be9MixedIntegerLinearProgram, GenericBackend: Add `__deepcopy__` methods

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 17, 2016

comment:7

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

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe changed the title Add copy/__copy__ methods to CVXOPT, PPL, InteractiveLP backends Add copy/__copy__ methods to CVXOPT, PPL, InteractiveLP backends, and __deepcopy__ to MixedIntegerLinearProgram and backends Apr 17, 2016
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 17, 2016

Author: Matthias Koeppe

@dimpase
Copy link
Member

dimpase commented Apr 18, 2016

comment:10

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 18, 2016

Changed commit from 38c6be9 to 3117b01

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 18, 2016

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

3117b01MixedIntegerLinearProgram.__deepcopy__: Add another doctest

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 18, 2016

comment:12

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.

@dimpase
Copy link
Member

dimpase commented Apr 18, 2016

comment:13

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?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 18, 2016

comment:14

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().

@dimpase
Copy link
Member

dimpase commented Apr 18, 2016

comment:15

ok, fine.

@dimpase
Copy link
Member

dimpase commented Apr 18, 2016

Reviewer: Dima Pasechnik

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 19, 2016

comment:16

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.

@vbraun
Copy link
Member

vbraun commented Apr 20, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants