Opened 3 years ago

Closed 3 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: sage-7.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 mkoeppe)

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

  • Description modified (diff)

comment:2 Changed 3 years ago by mkoeppe

  • Authors set to Matthias Koeppe
  • 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 3 years ago by mkoeppe

  • Branch set to u/mkoeppe/mixedintegerlinearprogram_gen___does_not_do_anything_useful

comment:4 Changed 3 years ago by mkoeppe

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

New commits:

434775dUpdate documentation
c54c656Update function index
dcbe23fMixedIntegerLinearProgram: Deprecate linear_function, __call__, and gen
72c8837MixedIntegerLinearProgram.show: Simplify, avoid calling gen
de373d0default_variable: New

comment:5 Changed 3 years ago by novoselt

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

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 3 years ago by chapoton

doc does not build + failing doctests, see patchbot report

OSError: [numerical] docstring of sage.numerical.mip:115:
WARNING: Inline interpreted text or phrase reference start-string
without end-string.

comment:8 Changed 3 years ago by git

  • Commit changed from de373d0d1159f917a15c6a6cf68954d83c668a1f to a1a1075ac97cacae334967278862a710bd1d3fa8

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

a1a1075Fix documentation markup and add deprecation info

comment:9 Changed 3 years ago by chapoton

Still 2 failing doctests

comment:10 Changed 3 years ago by git

  • Commit changed from a1a1075ac97cacae334967278862a710bd1d3fa8 to 684e91cad780b7dd2b0ec04e514b5ecf9ec95af0

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

987a47eUpdate documentation
71ebeaeUpdate function index
e427cccMixedIntegerLinearProgram: Deprecate linear_function, __call__, and gen
422abf6MixedIntegerLinearProgram.show: Simplify, avoid calling gen
881d1acdefault_variable: New
77beb8fFix documentation markup and add deprecation info
6375b4cLinearTensor: Add tests for hash and cmp
684e91cLinearFunction, LinearConstraint: Use linear_functions_parent() instead of deprecated MixedIntegerLinearProgram methods

comment:11 Changed 3 years ago by mkoeppe

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

ready for review.

comment:13 follow-up: Changed 3 years ago by 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.

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

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 3 years ago by chapoton

patchbot is green (the 4 failures are not related to this ticket)

comment:16 Changed 3 years ago by dimpase

  • Status changed from needs_review to positive_review

OK, looks good.

comment:17 follow-up: Changed 3 years ago by mkoeppe

  • Reviewers set to Dima Pasechnik

comment:18 in reply to: ↑ 17 Changed 3 years ago by dimpase

Replying to mkoeppe:

oh, right...

comment:19 Changed 3 years ago by vbraun

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