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

Deprecate MixedIntegerLinearProgram.gen(), __call__, linear_function, which do not do anything useful; add default_variable method #20602

Closed
mkoeppe opened this issue May 13, 2016 · 23 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented May 13, 2016

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.

CC: @nbruin @dimpase @videlec @jdemeyer @fchapoton @novoselt

Component: numerical

Author: Matthias Koeppe

Branch/Commit: 684e91c

Reviewer: Dima Pasechnik

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

@mkoeppe mkoeppe added this to the sage-7.3 milestone May 13, 2016
@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 27, 2016

Author: Matthias Koeppe

@mkoeppe mkoeppe changed the title MixedIntegerLinearProgram.gen() does not do anything useful Deprecate MixedIntegerLinearProgram.gen(), __call__, linear_function, which do not do anything useful; add default_variable method Jun 27, 2016
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 27, 2016

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 27, 2016

New commits:

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 27, 2016

Commit: de373d0

@novoselt
Copy link
Member

comment:5

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 27, 2016

comment:6

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.

@fchapoton
Copy link
Contributor

comment:7

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 27, 2016

Changed commit from de373d0 to a1a1075

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 27, 2016

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

a1a1075Fix documentation markup and add deprecation info

@fchapoton
Copy link
Contributor

comment:9

Still 2 failing doctests

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 7, 2016

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 7, 2016

Changed commit from a1a1075 to 684e91c

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 7, 2016

comment:11

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.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 11, 2016

comment:12

ready for review.

@dimpase
Copy link
Member

dimpase commented Jul 11, 2016

comment:13

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.

@fchapoton
Copy link
Contributor

comment:14

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

@fchapoton
Copy link
Contributor

comment:15

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

@dimpase
Copy link
Member

dimpase commented Jul 15, 2016

comment:16

OK, looks good.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 15, 2016

Reviewer: Dima Pasechnik

@dimpase
Copy link
Member

dimpase commented Jul 15, 2016

comment:18

Replying to @mkoeppe:

oh, right...

@vbraun
Copy link
Member

vbraun commented Jul 16, 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

5 participants