Deprecations: default LP variables will become real instead of nonnegative #15521

nathanncohen · 2013-12-15T18:58:06Z

Following the discussion on Sage-devel [1], default LP variables will become "real" instead of nonnegative as it is the case at the moment.

The aim is to have 4 types available through new_variable() : binary, integer, nonnegative, and real. Right now, nonnegative does not exist, and "real" represents nonnegative variables.

What this ticket does:

A warning has to be displayed when new_variable() is called without arguments, as the default behaviour will change. Users are advised to add nonnegative=True instead.
A warning has to be displayed when new_variable(real=True) is called, for it represents nonnegative variables at the moment and will represent real variables later. Users are advised to switch to nonnegative=True instead.

Nathann

[1] https://groups.google.com/d/msg/sage-devel/3vrPzUqFpM4/hKFp0RjV8poJ

Depends on #15489

CC: @dimpase @vbraun @dcoudert @sagetrac-hartke @wdjoyner @johnperry-math @benjaminfjones

Component: linear programming

Author: Nathann Cohen

Branch: 980c202

Reviewer: Benjamin Jones

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

The text was updated successfully, but these errors were encountered:

nathanncohen · 2013-12-16T16:49:11Z

Changed dependencies from #15482 to #15489

nathanncohen · 2013-12-16T16:49:11Z

comment:1

Heeeeeeere it is ! Warnings are displayed for each behaviour that will change in one year, and then we'll have something that makes sense AND does not lead anybody into a wall :-)

Nathann

nathanncohen · 2013-12-16T16:53:58Z

Branch: u/ncohen/15521

sagetrac-git · 2013-12-16T16:54:57Z

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

`[1b20b9f](https://github.com/sagemath/sagetrac-mirror/commit/1b20b9f)`	`trac #15521: Deprecations: default LP variables will become real instead of nonnegative`
`[eeb4e8c](https://github.com/sagemath/sagetrac-mirror/commit/eeb4e8c)`	`trac #15489: fix doctests and adds notes to remove code later`
`[8a901ae](https://github.com/sagemath/sagetrac-mirror/commit/8a901ae)`	`trac #15489: rebase on 5.13.rc0`
`[2156b3a](https://github.com/sagemath/sagetrac-mirror/commit/2156b3a)`	`trac #15489: deprecate the dim argument for MIPVariables`
`[9a66631](https://github.com/sagemath/sagetrac-mirror/commit/9a66631)`	`trac #15482: Scream where I had not thought of screaming before`
`[b50d7af](https://github.com/sagemath/sagetrac-mirror/commit/b50d7af)`	`trac #15482: addressing the reviewer's comments`
`[1f6c156](https://github.com/sagemath/sagetrac-mirror/commit/1f6c156)`	`trac #15482: Say very loud that LP variables are positive by default`

sagetrac-git · 2013-12-16T16:54:57Z

Commit: 1b20b9f

benjaminfjones · 2014-01-21T05:43:53Z

comment:5

Just having a look at this. All the cleanup is a nice addition along with the deprecation warning.

In the diff around @@ -577,6 +584,14 @@ cdef class MixedIntegerLinearProgram(SageObject):

There is a new test labeled "Default behavior" that I don't quite see the point of. Was your intent just to document the current default behavior and have this test change when the deprecation period is up?

nathanncohen · 2014-01-21T08:02:17Z

comment:6

Helloooooooo !!!

Just having a look at this. All the cleanup is a nice addition along with the deprecation warning.

Yep, but it did not apply on the latest 6.1.beta6. I just updated it :-)

In the diff around @@ -577,6 +584,14 @@ cdef class MixedIntegerLinearProgram(SageObject):

There is a new test labeled "Default behavior" that I don't quite see the point of. Was your intent just to document the current default behavior and have this test change when the deprecation period is up?

Hmmm... Yep, looks like this one is just there to check that the patch had his effect, a bit like how we add a doctest when we fix a bug. I know I added some doctests just to bring attention to the method when the default behaviour will be changed, but this one looks innocent ;-)

Nathann

sagetrac-git · 2014-01-21T08:08:49Z

Changed commit from 1b20b9f to c284c64

sagetrac-git · 2014-01-21T08:08:49Z

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

`18554c7`	`trac #15521: Rebase on 6.1.beta6`
`c284c64`	`trac #15521: Fixing the docstests reported by the patchbot`

nathanncohen · 2014-01-21T08:10:29Z

comment:8

By the way it would be cool you could review #15489 and possibly this ticket too, for otherwise writing patches changes nothing to Sage's actual code :-P

Nathann

benjaminfjones · 2014-01-23T19:09:59Z

comment:9

I can review #15489. Looks like it depends on #15482 (which has failing merge and doctests?) in turn.

On this ticket, your latest changes look good to me. I'm waiting for my local copy to build and test. How does the patchbot get triggered these days? Your latest commits didn't seem to get tested..

benjaminfjones · 2014-01-25T01:15:27Z

comment:10

I get 5 doctest failures from knapsack.py that all look like:

Failed example:
    knapsack( [(1,2), (1.5,1), (0.5,3)], max=2)
Exception raised:
    Traceback (most recent call last):
      File "/projects/9ac5a8ca-7dbf-4b27-adea-4709b0fb7105/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 48
0, in _run
        self.execute(example, compiled, test.globs)
      File "/projects/9ac5a8ca-7dbf-4b27-adea-4709b0fb7105/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 83
9, in execute
        exec compiled in globs
      File "<doctest sage.numerical.knapsack[1]>", line 1, in <module>
        knapsack( [(Integer(1),Integer(2)), (RealNumber('1.5'),Integer(1)), (RealNumber('0.5'),Integer(3))], max=Integer(2))
      File "/projects/9ac5a8ca-7dbf-4b27-adea-4709b0fb7105/sage/local/lib/python2.7/site-packages/sage/numerical/knapsack.py", lin
e 633, in knapsack
        p = MixedIntegerLinearProgram(solve=solver, maximization=True)
      File "mip.pyx", line 247, in sage.numerical.mip.MixedIntegerLinearProgram.__init__ (sage/numerical/mip.c:1692)
    TypeError: __init__() got an unexpected keyword argument 'solve'

sagetrac-git · 2014-01-28T19:48:34Z

Changed commit from c284c64 to 6b65d78

sagetrac-git · 2014-01-28T19:48:34Z

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

`ba762c8`	`trac #15521: Rebase on 6.1.rc1`
`6b65d78`	`trac #15521: A typo`

nathanncohen · 2014-01-28T19:48:47Z

comment:12

Sorryyyyyy ! It is now fixed.

Nathann

benjaminfjones · 2014-02-05T19:50:01Z

Reviewer: benjaminfjones

benjaminfjones · 2014-02-05T19:50:01Z

comment:14

Ok, back from hiatus. Your latest rebase and minor changes look good and tests are passing now. I'll give a positive review and have a look at those other tickets you mentioned.

nathanncohen · 2014-02-06T11:00:23Z

comment:15

Thaaaaaanks !! I just rebased #15482 which is a dependency of this ticket.

Nathann

vbraun · 2014-03-05T09:40:42Z

Changed reviewer from benjaminfjones to Benjamin Jones

vbraun · 2014-03-05T09:40:42Z

comment:16

Please use full names for the !Author/Reviewer fields next time

vbraun · 2014-03-05T09:44:38Z

comment:17

Conflict, please merge in latest beta

sagetrac-git · 2014-03-05T09:48:14Z

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

`e4acecb`	`trac #15521: Rebase on 6.2.beta3`

sagetrac-git · 2014-03-05T09:48:14Z

Changed commit from 6b65d78 to e4acecb

dimpase · 2014-03-05T15:58:46Z

comment:19

I am getting an error while building the relevant docs:

OSError: [numerical] docstring of sage.numerical.mip.MixedIntegerLinearProgram.set_binary:27: WARNING: Literal block expected; none found.

nathanncohen · 2014-03-05T16:13:04Z

comment:20

it is the same that appeared in #15489. So it will not happen when #15489 is applied.

Nathann

dimpase · 2014-03-05T16:22:57Z

comment:21

Replying to @nathanncohen:

it is the same that appeared in #15489. So it will not happen when #15489 is applied.

Huh? Are we back to the hg-like patch quilt? How come "#15489 is not applied" when I check this one out?

EDIT: I don't see how to make this consistent without a manual rebase: if I "apply" #15489 by checking it out, this "application" will be reversed upon checking out this ticket, isn't it?

nathanncohen · 2014-03-05T16:37:24Z

comment:22

Huh? Are we back to the hg-like patch quilt? How come "#15489 is not applied" when I check this one out?

git does not manage dependencies based on branch names. This "branch" does not depend on branch "15489". The first commit of this branch depends on the last commit of branch 15489 when it was created, but since that time I fixed the problem by adding another commit at the end of branch 15489, which is totally unrelated to this branch.

Thus, when you apply this branch, only the commits that actually depend on it are applied. Not the commit that were added later on that branch. Then again, git does not do anything based on branch names. Commit is the only thing that it sees.

EDIT: I don't see how to make this consistent without a manual rebase: if I "apply" #15489 by checking it out, this "application" will be reversed upon checking out this ticket, isn't it?

Indeed. If you want both, what you should do is :

Pull is equivalent to "fetch + merge". So it will merge this branch atop of #15489. This way everything that is currently in #15489 will coexist with everything which is in #15521. This is what Volker does when he builds a release : he pulls patches one after the other starting from the previous release.

Note that there is a "clean" way out : I can checkout this branch, pull #15489 (which will merge with this branch all commits from #15489 which are not already part of it) then update this branch with it. It's just that my Sage is compiling at the moment and I can't do it right now, so if you are satisfied with this explanation... :-P

Nathann

dimpase · 2014-03-05T17:56:25Z

comment:23

Replying to @nathanncohen:

EDIT: I don't see how to make this consistent without a manual rebase: if I "apply" #15489 by checking it out, this "application" will be reversed upon checking out this ticket, isn't it?

Indeed. If you want both, what you should do is :

checkout deprecate the dim argument for MIPVariables #15489

pull Deprecations: default LP variables will become real instead of nonnegative #15521

this is what I get if I try this at branch 15489:


$ git merge 15521
Auto-merging src/sage/numerical/mip.pyx
Auto-merging src/sage/graphs/generic_graph.py
CONFLICT (content): Merge conflict in src/sage/graphs/generic_graph.py
Removing src/sage/combinat/sf/kschur.py
Removing build/pkgs/gap/patches/cflags.patch
Removing build/pkgs/cython/patches/gdbout.patch
Removing build/pkgs/cython/patches/build_dir.patch
Automatic merge failed; fix conflicts and then commit the result.

looks like an automatic clean merge won't fly...

sagetrac-git · 2014-03-05T18:00:08Z

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

`6080660`	`trac #15489: Rebase on updated #15482`
`33d5c9f`	`trac #15489: Rebase on 6.2.beta2`
`cb4e41b`	`trac #15489: Docfix`
`980c202`	`trac #15521: Rebase on most recent version of #15489`

sagetrac-git · 2014-03-05T18:00:08Z

Changed commit from e4acecb to 980c202

nathanncohen · 2014-03-05T18:00:43Z

comment:25

Done

Nathann

dimpase · 2014-03-07T10:31:27Z

comment:26

Looks good; tested with sage --dev scripts

Just in case, note that somehow I was not able to make this to work using git the hard way, as I was getting bizarre docbuild errors, cf. my laments on sage-release regarding 6.2.beta3. (Perhaps it mattered that my system-wide git is older than the one packaged with Sage - this is one thing I didn't have time to check).

nathanncohen · 2014-03-07T12:17:09Z

comment:27

(but also probably because I slightly rewrote history once or twice, in order to change a commit message for instance)

Thanks for the review !

Nathann

dimpase · 2014-03-07T13:27:53Z

comment:28

Replying to @nathanncohen:

(but also probably because I slightly rewrote history once or twice, in order to change a commit message for instance)

hopefully this won't cause problems when merging into the release.

nathanncohen · 2014-03-07T13:30:41Z

comment:29

Nono it does not. There is nothing "wrong" with changing history. The only bad thing that happens is when you update some commits which have already been used by somebody else in another branch. You end up with having two commits which are meant to do the same thing, one of them sligthly modified (a different commit message, for instance). That is a problem.

But you can rewrite history 10000 times on your own computer before pushing the branch, in the same way that we can rewrite history 10000 times on this ticket if we are confident that nobody used the commits before we changed them.

Nathann

vbraun · 2014-03-13T02:38:42Z

Changed branch from u/ncohen/15521 to 980c202

jdemeyer · 2014-06-20T15:11:25Z

Changed commit from 980c202 to none

jdemeyer · 2014-06-20T15:11:25Z

comment:31

See #16504 for a follow-up.

jdemeyer · 2015-11-04T16:32:56Z

comment:32

Actually make the change: #19522

nathanncohen mannequin added this to the sage-6.1 milestone Dec 15, 2013

nathanncohen mannequin added c: linear programming labels Dec 15, 2013

nathanncohen mannequin added the s: needs review label Dec 16, 2013

sagetrac-vbraun-spam mannequin modified the milestones: sage-6.1, sage-6.2 Jan 30, 2014

benjaminfjones added s: positive review and removed s: needs review labels Feb 5, 2014

sagetrac-git mannequin added s: needs review and removed s: positive review labels Mar 5, 2014

dimpase added s: positive review and removed s: needs review labels Mar 7, 2014

vbraun removed the s: positive review label Mar 13, 2014

vbraun closed this as completed in c88356c Mar 13, 2014

jdemeyer mentioned this issue Nov 6, 2015

Change default for MILP variables to nonnegative=False #19522

Closed

Deprecations: default LP variables will become real instead of nonnegative #15521

Deprecations: default LP variables will become real instead of nonnegative #15521

Comments

nathanncohen mannequin commented Dec 15, 2013

nathanncohen mannequin commented Dec 16, 2013

nathanncohen mannequin commented Dec 16, 2013

nathanncohen mannequin commented Dec 16, 2013

sagetrac-git mannequin commented Dec 16, 2013

sagetrac-git mannequin commented Dec 16, 2013

benjaminfjones commented Jan 21, 2014

nathanncohen mannequin commented Jan 21, 2014

sagetrac-git mannequin commented Jan 21, 2014

sagetrac-git mannequin commented Jan 21, 2014

nathanncohen mannequin commented Jan 21, 2014

benjaminfjones commented Jan 23, 2014

benjaminfjones commented Jan 25, 2014

sagetrac-git mannequin commented Jan 28, 2014

sagetrac-git mannequin commented Jan 28, 2014

nathanncohen mannequin commented Jan 28, 2014

benjaminfjones commented Feb 5, 2014

benjaminfjones commented Feb 5, 2014

nathanncohen mannequin commented Feb 6, 2014

vbraun commented Mar 5, 2014

vbraun commented Mar 5, 2014

vbraun commented Mar 5, 2014

sagetrac-git mannequin commented Mar 5, 2014

sagetrac-git mannequin commented Mar 5, 2014

dimpase commented Mar 5, 2014

nathanncohen mannequin commented Mar 5, 2014

dimpase commented Mar 5, 2014

nathanncohen mannequin commented Mar 5, 2014

dimpase commented Mar 5, 2014

sagetrac-git mannequin commented Mar 5, 2014

sagetrac-git mannequin commented Mar 5, 2014

nathanncohen mannequin commented Mar 5, 2014

dimpase commented Mar 7, 2014

nathanncohen mannequin commented Mar 7, 2014

dimpase commented Mar 7, 2014

nathanncohen mannequin commented Mar 7, 2014

vbraun commented Mar 13, 2014

jdemeyer commented Jun 20, 2014

jdemeyer commented Jun 20, 2014

jdemeyer commented Nov 4, 2015