Opened 3 years ago
Closed 3 years ago
#20461 closed defect (fixed)
Fixes for copying a MIP and its variables
Reported by:  mkoeppe  Owned by:  

Priority:  major  Milestone:  sage7.3 
Component:  numerical  Keywords:  lp 
Cc:  dimpase, vdelecroix, jdemeyer, nbruin, chapoton  Merged in:  
Authors:  Matthias Koeppe  Reviewers:  Dima Pasechnik 
Report Upstream:  N/A  Work issues:  
Branch:  dff27e5 (Commits)  Commit:  dff27e5fca6197faf3747c100d493bc1b4908878 
Dependencies:  Stopgaps: 
Description (last modified by )
Even after merging #20414, both copy
and (the identical) deepcopy
have weird semantics in relation to the MIPVariable
s in a problems  because a MIP does not have an API to gain access to its variables; and trying to use the old MIPVariable
s with the copy is questionable and definitely broken if one creates new components of this variable, see #15159, #19523. Same when there's no explicit MIPVariable
and only the _default_mipvariable
is used via the problem's __getitem__
method:
sage: sage: p = MixedIntegerLinearProgram() sage: p[0] x_0 sage: q = copy(p) sage: q[0] x_0 sage: q[1] x_1 sage: sage: p.number_of_variables() 2 sage: sage: q.number_of_variables() 1
Here's a possible fix without having to change the whole design:
 a new
MixedIntegerLinearProgram.copy
method that takes anames
keyword argument, enabling this operation:sage: p.<x,y> = MixedIntegerLinearProgram() sage: q.<newx,newy> = p.copy()
and the less magical syntax
sage: q, newx, newy = p.copy([x, y])
copy
and__copy__
(and__deepcopy__
) should make a copy of _default_variable (this requires writing a__copy__
method forMIPVariable
) if
MixedIntegerLinearProgram.new_variable
has been called, it should set a flag and then if__copy__
(or__deepcopy__
) are called, it should display a warning (deprecation??) and refer the user to the newcopy
method.
In this ticket, we take care of the default MIP variable.
#20657 will take care of the variables created with new_variable
.
Change History (22)
comment:1 Changed 3 years ago by
 Description modified (diff)
comment:2 Changed 3 years ago by
comment:3 Changed 3 years ago by
 Description modified (diff)
comment:4 Changed 3 years ago by
sage: p = MixedIntegerLinearProgram() sage: x = p.new_variable(nonnegative=True) sage: x[0] x_0 sage: x[1] x_1 sage: p[0] #I don't understand this x_2 sage: p[1] x_3
But if we actually use the "generator" interface things seem OK:
sage: [p.gen(i) for i in [0..3]] [x_0, x_1, x_2, x_3]
so I would suggest to just make sure that p.gen(...)
works properly and indeed gives a variable of the appropriate MIP object. Then there IS an interface to get access to its variables (and really, it seems to be largely in place already) and then simply that can be used.
It looks like a variable on p is uniquely determined by its index (certainly its print name is), and p.new_variable
seems just a funny way of obtaining such indices.
The rest of the variable data is all hanging off internal tables, and via the "is_variable_*" methods you can obtain the type of the variables.
I think you first need to make sure that all of that is accessible (and copied appropriately, but I guess that's taken care of now) and then you can think about what to do with this "new_variable" business.
comment:5 Changed 3 years ago by
If one just uses gen()
. then everything already works as expected.
What is broken with copy
is the funny stuff involving MIPVariable
. This is what this ticket addresses.
comment:6 Changed 3 years ago by
Actually I spoke too fast, it seems that gen()
does not do anything useful.
sage: mip = MixedIntegerLinearProgram(solver='GLPK') sage: mip.gen(0) ### Names a variable, but does not create it in the backend x_0 sage: mip.number_of_variables() 0 sage: mip[0] ### This now creates a variable. It prints the same as the result of gen(0), but is different. x_0 sage: mip.get_values(mip.gen(0)) [...] TypeError: Not a MIPVariable: x_0 sage: mip.is_real(mip.gen(0)) [...] KeyError: x_0 sage: mip.is_real(mip[0]) True
comment:7 Changed 3 years ago by
Repairing gen()
is now #20602.
comment:8 Changed 3 years ago by
 Description modified (diff)
 Milestone changed from sage7.2 to sage7.3
comment:9 Changed 3 years ago by
 Branch set to u/mkoeppe/mip_copy_variables
comment:10 Changed 3 years ago by
 Commit set to fdd5c18c2ee341b230524cb3a7a539df0c902e3a
 Status changed from new to needs_review
New commits:
ed4082f  MIPVariable: Remove unused attributes

373aff4  MixedIntegerLinearProgram: Documentation fixes

68305b0  MixedIntegerLinearProgram.__getitem__: Simplify attribute access

f696ba1  MIPVariable.copy_for_mip: New

8edd4d8  MixedIntegerLinearProgram.__copy__: Copy the default MIP variable.

03b34b1  MixedIntegerLinearProgram.show: Remove unused code

fdd5c18  MixedIntegerLinearProgram: Documentation fixes

comment:11 Changed 3 years ago by
 Cc nbruin chapoton added
comment:12 Changed 3 years ago by
Needs review.
comment:13 Changed 3 years ago by
Shouldn't copy_for_mip
actually become copy
?
Or there is a reason to keep copy
as well?
comment:14 Changed 3 years ago by
Currently there is no copy
.
It seems an unusual operation to make a copy of a MIPVariable for the same problem.
comment:15 Changed 3 years ago by
 Commit changed from fdd5c18c2ee341b230524cb3a7a539df0c902e3a to dff27e5fca6197faf3747c100d493bc1b4908878
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
f59295c  MIPVariable: Remove unused attributes

67d8af3  MixedIntegerLinearProgram: Documentation fixes

f230df9  MixedIntegerLinearProgram.__getitem__: Simplify attribute access

2fb5134  MIPVariable.copy_for_mip: New

689becb  MixedIntegerLinearProgram.__copy__: Copy the default MIP variable.

4927701  MixedIntegerLinearProgram.show: Remove unused code

af80960  MixedIntegerLinearProgram: Documentation fixes

dff27e5  MIPVariable.__copy__, __deepcopy_: New

comment:16 Changed 3 years ago by
But anyway I made __copy__
and __deepcopy__
methods. (& rebased)
comment:17 Changed 3 years ago by
ping?
comment:18 Changed 3 years ago by
 Status changed from needs_review to positive_review
OK, looks good to me.
comment:21 Changed 3 years ago by
 Status changed from needs_work to positive_review
comment:22 Changed 3 years ago by
 Branch changed from u/mkoeppe/mip_copy_variables to dff27e5fca6197faf3747c100d493bc1b4908878
 Resolution set to fixed
 Status changed from positive_review to closed
Any thoughts on this proposal?