Deprecations: default LP variables will become real instead of nonnegative

[ncohen]

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

[ncohen]

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

[git]

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

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

[benjaminfjones]

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?

[ncohen]

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

[git]

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

[ncohen]

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]

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]

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'

[git]

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

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

[ncohen]

Sorryyyyyy ! It is now fixed.

Nathann

[benjaminfjones]

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.

[ncohen]

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

Nathann

[vbraun]

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

[vbraun]

Conflict, please merge in latest beta

[git]

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

[dimpase]

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.

[ncohen]

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

Nathann

[dimpase]

Replying to ncohen:

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?

[ncohen]

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 : 1) checkout #15489 2) pull #15521

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]

Replying to ncohen:

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 : 1) checkout #15489 2) pull #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...

[git]

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

[ncohen]

Done

Nathann

[dimpase]

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

[ncohen]

(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]

Replying to ncohen:

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

[ncohen]

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

[jdemeyer]

See #16504 for a follow-up.

[jdemeyer]

Actually make the change: #19522