Opened 4 years ago
Closed 4 years ago
#20602 closed defect (fixed)
Deprecate MixedIntegerLinearProgram.gen(), __call__, linear_function, which do not do anything useful; add default_variable method
Reported by:  mkoeppe  Owned by:  

Priority:  major  Milestone:  sage7.3 
Component:  numerical  Keywords:  lp 
Cc:  nbruin, dimpase, vdelecroix, jdemeyer, chapoton, novoselt  Merged in:  
Authors:  Matthias Koeppe  Reviewers:  Dima Pasechnik 
Report Upstream:  N/A  Work issues:  
Branch:  684e91c (Commits)  Commit:  684e91cad780b7dd2b0ec04e514b5ecf9ec95af0 
Dependencies:  Stopgaps: 
Description (last modified by )
As observed in the comments in #20461:
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
To summarize, the gen
method pretends that it can refer to backend variables (and so do linear_function
and the special __call__
method that is identical to linear_function
). In reality, these methods cannot be use for anything except for what is tested in the docstring: printing some meaningless stuff.
This patch deprecates these three methods and removes the corresponding confusing and useless examples from the class documentation.
In return, the notion of the "default MIP variable" (which __getitem__
refers to) is explained; and it is exposed to the user by new method default_variable
.
Change History (19)
comment:1 Changed 4 years ago by
 Description modified (diff)
comment:2 Changed 4 years ago by
 Cc chapoton novoselt added
 Description modified (diff)
 Summary changed from MixedIntegerLinearProgram.gen() does not do anything useful to Deprecate MixedIntegerLinearProgram.gen(), __call__, linear_function, which do not do anything useful; add default_variable method
comment:3 Changed 4 years ago by
 Branch set to u/mkoeppe/mixedintegerlinearprogram_gen___does_not_do_anything_useful
comment:4 Changed 4 years ago by
 Commit set to de373d0d1159f917a15c6a6cf68954d83c668a1f
 Status changed from new to needs_review
comment:5 Changed 4 years ago by
If the old methods were indeed always broken and could not be used for anything useful, I say  just delete (or fix) them instead of deprecating...
comment:6 Changed 4 years ago by
It's possible that there's some user code around that uses these methods as part of a workaround for the many bugs that had to be fixed in MixedIntegerLinearProgram
(see #20302 for a list). That's why I wanted to go the deprecation route with these methods.
comment:7 Changed 4 years ago by
doc does not build + failing doctests, see patchbot report
OSError: [numerical] docstring of sage.numerical.mip:115: WARNING: Inline interpreted text or phrase reference startstring without endstring.
comment:8 Changed 4 years ago by
 Commit changed from de373d0d1159f917a15c6a6cf68954d83c668a1f to a1a1075ac97cacae334967278862a710bd1d3fa8
Branch pushed to git repo; I updated commit sha1. New commits:
a1a1075  Fix documentation markup and add deprecation info

comment:9 Changed 4 years ago by
Still 2 failing doctests
comment:10 Changed 4 years ago by
 Commit changed from a1a1075ac97cacae334967278862a710bd1d3fa8 to 684e91cad780b7dd2b0ec04e514b5ecf9ec95af0
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
987a47e  Update documentation

71ebeae  Update function index

e427ccc  MixedIntegerLinearProgram: Deprecate linear_function, __call__, and gen

422abf6  MixedIntegerLinearProgram.show: Simplify, avoid calling gen

881d1ac  default_variable: New

77beb8f  Fix documentation markup and add deprecation info

6375b4c  LinearTensor: Add tests for hash and cmp

684e91c  LinearFunction, LinearConstraint: Use linear_functions_parent() instead of deprecated MixedIntegerLinearProgram methods

comment:11 Changed 4 years ago by
I've changed the doctests to no longer use the deprecated methods.
While doing this, I noticed that LinearTensor
had some doctests that were copied from LinearFunction
and were not testing the methods; and one of the methods needed fixing; done.
comment:12 Changed 4 years ago by
ready for review.
comment:13 followup: ↓ 14 Changed 4 years ago by
patchbots report nonsense; errors in a line that does not exist in the file src/sage/coding/guava.py
. How can this happen, I have no clue.
comment:14 in reply to: ↑ 13 Changed 4 years ago by
Replying to dimpase:
patchbots report nonsense; errors in a line that does not exist in the file
src/sage/coding/guava.py
. How can this happen, I have no clue.
The patchbot report was for 7.3.b6, and there was a real issue with guava, which has since been solved. I am launching my own bot on the ticket, but this bot has its own fake failures for the moment (solved in the next beta).
comment:15 Changed 4 years ago by
patchbot is green (the 4 failures are not related to this ticket)
comment:16 Changed 4 years ago by
 Status changed from needs_review to positive_review
OK, looks good.
comment:17 followup: ↓ 18 Changed 4 years ago by
 Reviewers set to Dima Pasechnik
comment:18 in reply to: ↑ 17 Changed 4 years ago by
Replying to mkoeppe:
oh, right...
comment:19 Changed 4 years ago by
 Branch changed from u/mkoeppe/mixedintegerlinearprogram_gen___does_not_do_anything_useful to 684e91cad780b7dd2b0ec04e514b5ecf9ec95af0
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Update documentation
Update function index
MixedIntegerLinearProgram: Deprecate linear_function, __call__, and gen
MixedIntegerLinearProgram.show: Simplify, avoid calling gen
default_variable: New