Say very loud that LP variables are non-negative by default #15482

nathanncohen · 2013-12-04T08:53:51Z

As the title says. It has been reported once again. We make this assumption because ALL lp solvers that we use make the same, but it does not mean everybody knows it T_T

Nathann

CC: @kcrisman

Component: documentation

Author: Nathann Cohen

Branch/Commit: 9a7657b

Reviewer: Punarbasu Purkayastha, Karl-Dieter Crisman, Thierry Monteil

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

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

nathanncohen · 2013-12-04T08:54:20Z

Branch: u/ncohen/15482

sagetrac-git · 2013-12-04T08:54:49Z

Commit: 1f6c156

sagetrac-git · 2013-12-04T08:54:49Z

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

`[1f6c156](https://github.com/sagemath/sagetrac-mirror/commit/1f6c156)`	`trac #15482: Say very loud that LP variables are positive by default`

ppurka · 2013-12-04T14:58:45Z

comment:3

The changes and cleanups (thanks!) look good to me.

Can you also put the warning in one additional place - the docstring for the class MixedIntegerLinearProgram? Since this is only documentation changes, I think with the additional change, it is good to go.

nathanncohen · 2013-12-04T15:10:41Z

comment:4

What do you think ? I added a warning and removed some lines which (I believe) did not help much and make the doc easier to read.

Nathann

sagetrac-git · 2013-12-04T15:12:12Z

Changed commit from 1f6c156 to b50d7af

sagetrac-git · 2013-12-04T15:12:12Z

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

`[b50d7af](https://github.com/sagemath/sagetrac-mirror/commit/b50d7af)`	`trac #15482: addressing the reviewer's comments`

ppurka · 2013-12-04T15:24:39Z

comment:6

Ok. Looks good to me now. Enough shouting. :)

ppurka · 2013-12-04T15:24:39Z

Reviewer: Punarbasu Purkayastha

kcrisman · 2013-12-04T16:13:52Z

comment:7

Great and fast work. But... sorry, not loud enough.

If you look at the LP tutorial, there is no indication of this (other than a parenthetical "for instance when you want some variables to be negative") - and that is where people will end with current SEO results. This doc does have it in the opening example, but even there it is not as visible as possible. I think that for positive review here, we really need to have a :warning or something like that in a few places. Remember, this is for people coming to LP who are not in all likelihood crazy graaaaaph theorists or in operations research, and who (like me, though I am not a complete novice to using LPs) will not at all suspect this problem.

And we all know that students only look for worked examples in order to do their homework, so why should it be any different with working people in the mathematical sciences? :-)

ppurka · 2013-12-04T16:27:23Z

comment:8

You are right - but should there be a "warning" in a tutorial? I think it should be a usual statement.

The MIP mentions it in the introductory part and then Nathann has already fixed it for the MILP part. Is there anywhere else in MIP that you have in mind?

nathanncohen · 2013-12-04T19:06:47Z

comment:9

Hmmmmmm... Right for the thematic tutorial, I am doing this right now. Right for the first example of the MIP module, though I did it in the first version of the patch I submitted to finally remove it, as adding a warning or actually a bold text more or less "broke" the flow of this explanation.

Hmmm.. I'll give it another try and add a commit to this ticket.

Nathann

sagetrac-git · 2013-12-04T20:01:42Z

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

`[9a66631](https://github.com/sagemath/sagetrac-mirror/commit/9a66631)`	`trac #15482: Scream where I had not thought of screaming before`

sagetrac-git · 2013-12-04T20:01:42Z

Changed commit from b50d7af to 9a66631

kcrisman · 2013-12-05T00:50:43Z

comment:12

I'm happy with the content of this as respects my earlier comment - thanks. I don't know what it looks like, and a little of the details have changed (what happened to dim?) I won't speak for the formatting but I'm sure ppurka will look at that carefully. Do note that the lines seem to be longer than usual, more than 80 characters, perhaps?

nathanncohen · 2013-12-05T08:36:07Z

comment:13

Oh, well I removed this "dim" thing and I should deprecate it anyway. I'll send a patch for that today. I wrote this "dim" thing when I had no idea that a[1,2] was a valid key for a dictionary, and what I wrote is not only ugly and heavy but even slower. I'm pretty sure nobody but me uses that (and I stopped using them years ago), and it really isn't a good thing to put in the tutorial anyay.

As per the lines, they should be 80 characters long if Emacs did its job O_o

Nathann

nathanncohen · 2013-12-06T12:27:21Z

comment:14

(just created #15489 for this dim problem)

Nathann

ppurka · 2013-12-06T13:06:59Z

comment:15

Ok. Looks good to me.

One paragraph (line 135...) has 80-87 characters long lines. Not a big deal for me. I think your emacs setting has 85 chars not 80. You can update the ticket if you want with reflowed lines.

ppurka · 2013-12-06T13:06:59Z

Changed reviewer from Punarbasu Purkayastha to Punarbasu Purkayastha, Karl-Dieter Crisman

nathanncohen · 2013-12-16T14:18:47Z

comment:26

Sorryyyyyyy guys, there was a broken doctest in graph.py because of the change to MixedIntegerLinearProgra.solve which removed an old argument. SOoooooo this thing is waiting for a review again ^^;

Nathann

sagetrac-git · 2013-12-16T14:59:29Z

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

`[c8addd9](https://github.com/sagemath/sagetrac-mirror/commit/c8addd9)`	`trac #15482: rebase on 5.13.rc0`

sagetrac-git · 2013-12-16T14:59:29Z

Changed commit from dd7bde8 to c8addd9

ppurka · 2013-12-21T13:33:07Z

comment:29

What is up with the hundreds of failing doctests? o_O

nathanncohen · 2013-12-21T14:22:12Z

comment:30

T_T

I knew something was weird. The doctests passed on my computer, and I was wondering if there was something new with the deprecation warnings, like they are "only shown once" in the first doctests of each file and never again. It all passed.

I guess it was a bug and all functions need to be fixed, which seems natural. I am recompiling the commit over beta1 and I will check it again. Hoping that the doctests fail on my computer, this time :-P

Nathann

nathanncohen · 2013-12-21T14:55:25Z

comment:31

Oh. Well. All tests pass in both the graph/ and numerical/ folder O_o

Soooooooo where are your broken doctests, actually ? O_o

Nathann

nathanncohen · 2013-12-21T14:57:04Z

comment:32

(sorry about my previous comment by the way. I was thinking of a different patch)

sagetrac-git · 2014-02-06T10:59:33Z

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

`076f85b`	`trac #15482: Rebase on 6.2.beta0`

sagetrac-git · 2014-02-06T10:59:33Z

Changed commit from c8addd9 to 076f85b

rwst · 2014-02-13T07:11:45Z

comment:35

Is this supposed to work on Sage-6.0? I get 70 doctests fails.

sagetrac-git · 2014-02-13T10:15:04Z

Changed commit from 076f85b to b87f1de

sagetrac-git · 2014-02-13T10:15:04Z

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

`b87f1de`	`trac #15482: Rebase on 6.2.beta1`

nathanncohen · 2014-02-13T10:18:29Z

comment:37

70 fails ? Where ? O_o

I just rebased it on 6.2.beta1 (which should change nothing at all), and well...

~/sage$ sage -b && sage -tp 4 -l graphs/ numerical/
...
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------

Nathann

ppurka · 2014-02-14T09:01:27Z

comment:38

I got one doctest failure:

Running doctests with ID 2014-02-14-16-32-46-c7933957.
Doctesting 1 file.
sage -t --long src/doc/en/thematic_tutorials/linear_programming.rst
**********************************************************************
File "src/doc/en/thematic_tutorials/linear_programming.rst", line 181, in doc.en.thematic_tutorials.linear_programming
Failed example:
    p.add_constraint(y[3,2] + x[5] == 6)
Exception raised:
    Traceback (most recent call last):
      File "/home/basu/Installations/sage-6.1.beta2.server/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 480, in _run
        self.execute(example, compiled, test.globs)
      File "/home/basu/Installations/sage-6.1.beta2.server/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 839, in execute
        exec compiled in globs
      File "<doctest doc.en.thematic_tutorials.linear_programming[12]>", line 1, in <module>
        p.add_constraint(y[Integer(3),Integer(2)] + x[Integer(5)] == Integer(6))
    TypeError: 'sage.numerical.linear_functions.LinearFunction' object has no attribute '__getitem__'
**********************************************************************
1 item had failures:
   1 of  48 in doc.en.thematic_tutorials.linear_programming
    [44 tests, 1 failure, 0.10 s]
----------------------------------------------------------------------
sage -t --long src/doc/en/thematic_tutorials/linear_programming.rst  # 1 doctest failed
----------------------------------------------------------------------
Total time for all tests: 0.2 seconds
    cpu time: 0.1 seconds
    cumulative wall time: 0.1 seconds

nathanncohen · 2014-02-14T10:52:05Z

comment:39

Oh. I fixed it already somewhere else, I am pretty sure. Anyway, some later patch will conflict :-D

Anyway, fixed again !

Nathann

sagetrac-git · 2014-02-14T10:52:17Z

Changed commit from b87f1de to 9a7657b

sagetrac-git · 2014-02-14T10:52:17Z

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

`9a7657b`	`trac #15482: Broken doctest`

ppurka · 2014-02-14T10:56:24Z

comment:41

Looks good. This process took inordinately long just for documentation changes. :(

nathanncohen · 2014-02-14T10:58:06Z

comment:42

Thaaaaaaaaanks !!! I will now rebase #15489, its branch name is red for some reason O_o

Nathann

rwst · 2014-02-14T10:59:44Z

comment:43

OK, confirm all tests passed, too. I'm slowly coming to grips with the system.

vbraun · 2014-02-21T20:07:58Z

Changed branch from u/ncohen/15482 to 9a7657b

nathanncohen mannequin added this to the sage-5.13 milestone Dec 4, 2013

nathanncohen mannequin added c: documentation labels Dec 4, 2013

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

ppurka added s: positive review and removed s: needs review labels Dec 4, 2013

kcrisman added s: needs work and removed s: positive review labels Dec 4, 2013

nathanncohen mannequin added s: needs review and removed s: needs work labels Dec 4, 2013

sagetrac-vbraun-spam mannequin modified the milestones: sage-6.0, sage-6.1 Dec 17, 2013

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

ppurka added s: positive review and removed s: needs review labels Feb 14, 2014

vbraun removed the s: positive review label Feb 21, 2014

vbraun closed this as completed in bc4d27f Feb 21, 2014

nathanncohen mannequin mentioned this issue Mar 5, 2014

deprecate the dim argument for MIPVariables #15489

Closed

Say very loud that LP variables are non-negative by default #15482

Say very loud that LP variables are non-negative by default #15482

Comments

nathanncohen mannequin commented Dec 4, 2013

nathanncohen mannequin commented Dec 4, 2013

sagetrac-git mannequin commented Dec 4, 2013

sagetrac-git mannequin commented Dec 4, 2013

ppurka commented Dec 4, 2013

nathanncohen mannequin commented Dec 4, 2013

sagetrac-git mannequin commented Dec 4, 2013

sagetrac-git mannequin commented Dec 4, 2013

ppurka commented Dec 4, 2013

ppurka commented Dec 4, 2013

kcrisman commented Dec 4, 2013

ppurka commented Dec 4, 2013

nathanncohen mannequin commented Dec 4, 2013

sagetrac-git mannequin commented Dec 4, 2013

sagetrac-git mannequin commented Dec 4, 2013

kcrisman commented Dec 5, 2013

nathanncohen mannequin commented Dec 5, 2013

nathanncohen mannequin commented Dec 6, 2013

ppurka commented Dec 6, 2013

ppurka commented Dec 6, 2013

nathanncohen mannequin commented Dec 16, 2013

sagetrac-git mannequin commented Dec 16, 2013

sagetrac-git mannequin commented Dec 16, 2013

ppurka commented Dec 21, 2013

nathanncohen mannequin commented Dec 21, 2013

nathanncohen mannequin commented Dec 21, 2013

nathanncohen mannequin commented Dec 21, 2013

sagetrac-git mannequin commented Feb 6, 2014

sagetrac-git mannequin commented Feb 6, 2014

rwst commented Feb 13, 2014

sagetrac-git mannequin commented Feb 13, 2014

sagetrac-git mannequin commented Feb 13, 2014

nathanncohen mannequin commented Feb 13, 2014

ppurka commented Feb 14, 2014

nathanncohen mannequin commented Feb 14, 2014

sagetrac-git mannequin commented Feb 14, 2014

sagetrac-git mannequin commented Feb 14, 2014

ppurka commented Feb 14, 2014

nathanncohen mannequin commented Feb 14, 2014

rwst commented Feb 14, 2014

vbraun commented Feb 21, 2014