Opened 4 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: sage-7.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 mkoeppe)

Even after merging #20414, both copy and (the identical) 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. 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 a names 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 for MIPVariable)
  • 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 new copy 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 4 years ago by mkoeppe

  • Description modified (diff)

comment:2 Changed 4 years ago by mkoeppe

Any thoughts on this proposal?

comment:3 Changed 4 years ago by mkoeppe

  • Description modified (diff)

comment:4 Changed 4 years ago by nbruin

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 4 years ago by mkoeppe

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 4 years ago by mkoeppe

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 4 years ago by mkoeppe

Repairing gen() is now #20602.

comment:8 Changed 3 years ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Description modified (diff)
  • Milestone changed from sage-7.2 to sage-7.3

comment:9 Changed 3 years ago by mkoeppe

  • Branch set to u/mkoeppe/mip_copy_variables

comment:10 Changed 3 years ago by mkoeppe

  • Commit set to fdd5c18c2ee341b230524cb3a7a539df0c902e3a
  • Status changed from new to needs_review

New commits:

ed4082fMIPVariable: Remove unused attributes
373aff4MixedIntegerLinearProgram: Documentation fixes
68305b0MixedIntegerLinearProgram.__getitem__: Simplify attribute access
f696ba1MIPVariable.copy_for_mip: New
8edd4d8MixedIntegerLinearProgram.__copy__: Copy the default MIP variable.
03b34b1MixedIntegerLinearProgram.show: Remove unused code
fdd5c18MixedIntegerLinearProgram: Documentation fixes

comment:11 Changed 3 years ago by mkoeppe

  • Cc nbruin chapoton added

comment:12 Changed 3 years ago by mkoeppe

Needs review.

comment:13 Changed 3 years ago by dimpase

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 mkoeppe

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 git

  • Commit changed from fdd5c18c2ee341b230524cb3a7a539df0c902e3a to dff27e5fca6197faf3747c100d493bc1b4908878

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

f59295cMIPVariable: Remove unused attributes
67d8af3MixedIntegerLinearProgram: Documentation fixes
f230df9MixedIntegerLinearProgram.__getitem__: Simplify attribute access
2fb5134MIPVariable.copy_for_mip: New
689becbMixedIntegerLinearProgram.__copy__: Copy the default MIP variable.
4927701MixedIntegerLinearProgram.show: Remove unused code
af80960MixedIntegerLinearProgram: Documentation fixes
dff27e5MIPVariable.__copy__, __deepcopy_: New

comment:16 Changed 3 years ago by mkoeppe

But anyway I made __copy__ and __deepcopy__ methods. (& rebased)

Last edited 3 years ago by mkoeppe (previous) (diff)

comment:17 Changed 3 years ago by mkoeppe

ping?

comment:18 Changed 3 years ago by dimpase

  • Status changed from needs_review to positive_review

OK, looks good to me.

comment:19 Changed 3 years ago by vbraun

  • Status changed from positive_review to needs_work

Reviewer name

comment:20 Changed 3 years ago by dimpase

  • Reviewers set to Dima Pasechnik

I will never learn the drill :-)

comment:21 Changed 3 years ago by dimpase

  • Status changed from needs_work to positive_review

comment:22 Changed 3 years ago by vbraun

  • Branch changed from u/mkoeppe/mip_copy_variables to dff27e5fca6197faf3747c100d493bc1b4908878
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.