Opened 6 years ago

Closed 5 years ago

Last modified 4 years ago

#15521 closed enhancement (fixed)

Deprecations: default LP variables will become real instead of nonnegative

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.2
Component: linear programming Keywords:
Cc: dimpase, vbraun, dcoudert, hartke, wdj, john_perry, benjaminfjones Merged in:
Authors: Nathann Cohen Reviewers: Benjamin Jones
Report Upstream: N/A Work issues:
Branch: 980c202 (Commits) Commit:
Dependencies: #15489 Stopgaps:

Description

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

Change History (32)

comment:1 Changed 6 years ago by ncohen

  • Dependencies changed from #15482 to #15489
  • Status changed from new to needs_review

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

comment:2 Changed 6 years ago by ncohen

  • Branch set to u/ncohen/15521
  • Cc dimpase vbraun dcoudert hartke wdj john_perry added

comment:3 Changed 6 years ago by git

  • Commit set to 1b20b9f3d5427d6b6ea58335e7bbf4dd74bd46c3

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

1b20b9ftrac #15521: Deprecations: default LP variables will become real instead of nonnegative
eeb4e8ctrac #15489: fix doctests and adds notes to remove code later
8a901aetrac #15489: rebase on 5.13.rc0
2156b3atrac #15489: deprecate the dim argument for MIPVariables
9a66631trac #15482: Scream where I had not thought of screaming before
b50d7aftrac #15482: addressing the reviewer's comments
1f6c156trac #15482: Say very loud that LP variables are positive by default

comment:4 Changed 6 years ago by benjaminfjones

  • Cc benjaminfjones added

comment:5 follow-up: Changed 5 years ago by 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?

comment:6 in reply to: ↑ 5 Changed 5 years ago by 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

comment:7 Changed 5 years ago by git

  • Commit changed from 1b20b9f3d5427d6b6ea58335e7bbf4dd74bd46c3 to c284c64f9b612de38373672e007638b350ba3212

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

18554c7trac #15521: Rebase on 6.1.beta6
c284c64trac #15521: Fixing the docstests reported by the patchbot

comment:8 Changed 5 years ago by 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

comment:9 Changed 5 years ago by 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..

comment:10 Changed 5 years ago by 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'

comment:11 Changed 5 years ago by git

  • Commit changed from c284c64f9b612de38373672e007638b350ba3212 to 6b65d78580c1625384c262b19fc2d4d04de67555

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

ba762c8trac #15521: Rebase on 6.1.rc1
6b65d78trac #15521: A typo

comment:12 Changed 5 years ago by ncohen

Sorryyyyyy ! It is now fixed.

Nathann

comment:13 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:14 Changed 5 years ago by benjaminfjones

  • Reviewers set to benjaminfjones
  • Status changed from needs_review to positive_review

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.

comment:15 Changed 5 years ago by ncohen

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

Nathann

comment:16 Changed 5 years ago by vbraun

  • Reviewers changed from benjaminfjones to Benjamin Jones

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

comment:17 Changed 5 years ago by vbraun

Conflict, please merge in latest beta

comment:18 Changed 5 years ago by git

  • Commit changed from 6b65d78580c1625384c262b19fc2d4d04de67555 to e4acecb4779921e094ffb1e403144f8924cfabf8
  • Status changed from positive_review to needs_review

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

e4acecbtrac #15521: Rebase on 6.2.beta3

comment:19 Changed 5 years ago by 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.

comment:20 follow-up: Changed 5 years ago by ncohen

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

Nathann

comment:21 in reply to: ↑ 20 ; follow-up: Changed 5 years ago by 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?

Last edited 5 years ago by dimpase (previous) (diff)

comment:22 in reply to: ↑ 21 ; follow-up: Changed 5 years ago by 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

comment:23 in reply to: ↑ 22 Changed 5 years ago by 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...

comment:24 Changed 5 years ago by git

  • Commit changed from e4acecb4779921e094ffb1e403144f8924cfabf8 to 980c2026f6f5928305b528c9bfa793c6faa84a50

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

6080660trac #15489: Rebase on updated #15482
33d5c9ftrac #15489: Rebase on 6.2.beta2
cb4e41btrac #15489: Docfix
980c202trac #15521: Rebase on most recent version of #15489

comment:25 Changed 5 years ago by ncohen

Done

Nathann

comment:26 Changed 5 years ago by dimpase

  • Status changed from needs_review to positive_review

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

Last edited 5 years ago by dimpase (previous) (diff)

comment:27 follow-up: Changed 5 years ago by 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

comment:28 in reply to: ↑ 27 Changed 5 years ago by 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.

comment:29 Changed 5 years ago by 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

comment:30 Changed 5 years ago by vbraun

  • Branch changed from u/ncohen/15521 to 980c2026f6f5928305b528c9bfa793c6faa84a50
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:31 Changed 5 years ago by jdemeyer

  • Commit 980c2026f6f5928305b528c9bfa793c6faa84a50 deleted

See #16504 for a follow-up.

comment:32 Changed 4 years ago by jdemeyer

Actually make the change: #19522

Note: See TracTickets for help on using tickets.