#15521 closed enhancement (fixed)
Deprecations: default LP variables will become real instead of nonnegative
Reported by:  ncohen  Owned by:  

Priority:  major  Milestone:  sage6.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 Sagedevel [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 addnonnegative=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 tononnegative=True
instead.
Nathann
[1] https://groups.google.com/d/msg/sagedevel/3vrPzUqFpM4/hKFp0RjV8poJ
Change History (32)
comment:1 Changed 6 years ago by
 Dependencies changed from #15482 to #15489
 Status changed from new to needs_review
comment:2 Changed 6 years ago by
 Branch set to u/ncohen/15521
 Cc dimpase vbraun dcoudert hartke wdj john_perry added
comment:3 Changed 6 years ago by
 Commit set to 1b20b9f3d5427d6b6ea58335e7bbf4dd74bd46c3
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 
comment:4 Changed 6 years ago by
 Cc benjaminfjones added
comment:5 followup: ↓ 6 Changed 5 years ago by
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
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
 Commit changed from 1b20b9f3d5427d6b6ea58335e7bbf4dd74bd46c3 to c284c64f9b612de38373672e007638b350ba3212
comment:8 Changed 5 years ago by
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
comment:10 Changed 5 years ago by
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/9ac5a8ca7dbf4b27adea4709b0fb7105/sage/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 48 0, in _run self.execute(example, compiled, test.globs) File "/projects/9ac5a8ca7dbf4b27adea4709b0fb7105/sage/local/lib/python2.7/sitepackages/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/9ac5a8ca7dbf4b27adea4709b0fb7105/sage/local/lib/python2.7/sitepackages/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
 Commit changed from c284c64f9b612de38373672e007638b350ba3212 to 6b65d78580c1625384c262b19fc2d4d04de67555
comment:12 Changed 5 years ago by
Sorryyyyyy ! It is now fixed.
Nathann
comment:13 Changed 5 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:14 Changed 5 years ago by
 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
Thaaaaaanks !! I just rebased #15482 which is a dependency of this ticket.
Nathann
comment:16 Changed 5 years ago by
 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
Conflict, please merge in latest beta
comment:18 Changed 5 years ago by
 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:
e4acecb  trac #15521: Rebase on 6.2.beta3

comment:19 Changed 5 years ago by
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 followup: ↓ 21 Changed 5 years ago by
comment:21 in reply to: ↑ 20 ; followup: ↓ 22 Changed 5 years ago by
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 hglike 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?
comment:22 in reply to: ↑ 21 ; followup: ↓ 23 Changed 5 years ago by
Huh? Are we back to the hglike 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
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 Automerging src/sage/numerical/mip.pyx Automerging 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
 Commit changed from e4acecb4779921e094ffb1e403144f8924cfabf8 to 980c2026f6f5928305b528c9bfa793c6faa84a50
comment:25 Changed 5 years ago by
Done
Nathann
comment:26 Changed 5 years ago by
 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 sagerelease regarding 6.2.beta3. (Perhaps it mattered that my systemwide git is older than the one packaged with Sage  this is one thing I didn't have time to check).
comment:27 followup: ↓ 28 Changed 5 years ago by
(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
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
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
 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
 Commit 980c2026f6f5928305b528c9bfa793c6faa84a50 deleted
See #16504 for a followup.
comment:32 Changed 4 years ago by
Actually make the change: #19522
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