Opened 9 years ago

Closed 9 years ago

#15482 closed defect (fixed)

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

Reported by: Nathann Cohen Owned by:
Priority: major Milestone: sage-6.2
Component: documentation Keywords:
Cc: Karl-Dieter Crisman Merged in:
Authors: Nathann Cohen Reviewers: Punarbasu Purkayastha, Karl-Dieter Crisman, Thierry Monteil
Report Upstream: N/A Work issues:
Branch: 9a7657b (Commits, GitHub, GitLab) Commit: 9a7657bb5ce501543b7359e7bf19764d2b11e3cc
Dependencies: Stopgaps:

Status badges

Description

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

Change History (44)

comment:1 Changed 9 years ago by Nathann Cohen

Branch: u/ncohen/15482
Status: newneeds_review

comment:2 Changed 9 years ago by git

Commit: 1f6c156dfd70534f26ddb2e639f57c040dc199fa

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

1f6c156trac #15482: Say very loud that LP variables are positive by default

comment:3 Changed 9 years ago by Punarbasu Purkayastha

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.

comment:4 Changed 9 years ago by Nathann Cohen

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

comment:5 Changed 9 years ago by git

Commit: 1f6c156dfd70534f26ddb2e639f57c040dc199fab50d7af25a94022efa29eccd3637bcd9396fab16

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

b50d7aftrac #15482: addressing the reviewer's comments

comment:6 Changed 9 years ago by Punarbasu Purkayastha

Reviewers: Punarbasu Purkayastha
Status: needs_reviewpositive_review

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

comment:7 Changed 9 years ago by Karl-Dieter Crisman

Status: positive_reviewneeds_work

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? :-)

comment:8 Changed 9 years ago by Punarbasu Purkayastha

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?

comment:9 Changed 9 years ago by Nathann Cohen

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

comment:10 Changed 9 years ago by git

Commit: b50d7af25a94022efa29eccd3637bcd9396fab169a666314188dc3681dea63922458096e6380ea41

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

9a66631trac #15482: Scream where I had not thought of screaming before

comment:11 Changed 9 years ago by Nathann Cohen

Status: needs_workneeds_review

comment:12 Changed 9 years ago by Karl-Dieter Crisman

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?

comment:13 Changed 9 years ago by Nathann Cohen

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

comment:14 Changed 9 years ago by Nathann Cohen

(just created #15489 for this dim problem)

Nathann

comment:15 Changed 9 years ago by Punarbasu Purkayastha

Reviewers: Punarbasu PurkayasthaPunarbasu Purkayastha, Karl-Dieter Crisman
Status: needs_reviewpositive_review

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.

comment:16 Changed 9 years ago by git

Commit: 9a666314188dc3681dea63922458096e6380ea41d2870e1a5a9d449ecabf1859f2d380308eb5dcab
Status: positive_reviewneeds_review

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

d2870e1trac #15482: shortening a line

comment:17 Changed 9 years ago by Nathann Cohen

Ahahaha. Okay okay, done. I love git commmits :-P

Nathann

comment:18 Changed 9 years ago by Nathann Cohen

Status: needs_reviewpositive_review

comment:19 Changed 9 years ago by Jeroen Demeyer

Milestone: sage-5.13sage-6.0

comment:20 Changed 9 years ago by Thierry Monteil

Status: positive_reviewneeds_info

In http://git.sagemath.org/sage.git/commit/?id=1f6c156 (sorry i didn't move to the git workflow yet), in the sentence "By default, all variables of a LP are assumed to be positive", shouldn't it be "non-negative" instead of "positive" (classical french mistake :p) ?

comment:21 Changed 9 years ago by git

Commit: d2870e1a5a9d449ecabf1859f2d380308eb5dcabcd5bb43f35adfc8c03c2893275aa8245d76b1204

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

cd5bb43trac #15482: positive -> non-negative

comment:22 Changed 9 years ago by Nathann Cohen

Status: needs_infopositive_review

Done.

comment:23 Changed 9 years ago by Thierry Monteil

Reviewers: Punarbasu Purkayastha, Karl-Dieter CrismanPunarbasu Purkayastha, Karl-Dieter Crisman, Thierry Monteil
Summary: Say very loud that LP variables are positive by defaultSay very loud that LP variables are non-negative by default

non-negative review :P

comment:24 in reply to:  23 Changed 9 years ago by Karl-Dieter Crisman

non-negative review :P

LOL

comment:25 Changed 9 years ago by git

Commit: cd5bb43f35adfc8c03c2893275aa8245d76b1204dd7bde803644daee20e4b0439914814eb1cf48cb
Status: positive_reviewneeds_review

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

dd7bde8trac #15482: a broken doctest

comment:26 Changed 9 years ago by Nathann Cohen

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

comment:27 Changed 9 years ago by git

Commit: dd7bde803644daee20e4b0439914814eb1cf48cbc8addd909e463c56949babef9804f603742381dc

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

c8addd9trac #15482: rebase on 5.13.rc0

comment:28 Changed 9 years ago by For batch modifications

Milestone: sage-6.0sage-6.1

comment:29 Changed 9 years ago by Punarbasu Purkayastha

What is up with the hundreds of failing doctests? o_O

comment:30 Changed 9 years ago by Nathann Cohen

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

comment:31 Changed 9 years ago by Nathann Cohen

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

Soooooooo where are your broken doctests, actually ? O_o

Nathann

comment:32 Changed 9 years ago by Nathann Cohen

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

comment:33 Changed 9 years ago by For batch modifications

Milestone: sage-6.1sage-6.2

comment:34 Changed 9 years ago by git

Commit: c8addd909e463c56949babef9804f603742381dc076f85bd75e7f69706bf4bd238a8a50f8d180bc0

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

076f85btrac #15482: Rebase on 6.2.beta0

comment:35 Changed 9 years ago by Ralf Stephan

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

comment:36 Changed 9 years ago by git

Commit: 076f85bd75e7f69706bf4bd238a8a50f8d180bc0b87f1deb520451cc8d770c9b3e4f9268070912b8

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

b87f1detrac #15482: Rebase on 6.2.beta1

comment:37 Changed 9 years ago by Nathann Cohen

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

comment:38 Changed 9 years ago by Punarbasu Purkayastha

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

comment:39 Changed 9 years ago by Nathann Cohen

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

Anyway, fixed again !

Nathann

comment:40 Changed 9 years ago by git

Commit: b87f1deb520451cc8d770c9b3e4f9268070912b89a7657bb5ce501543b7359e7bf19764d2b11e3cc

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

9a7657btrac #15482: Broken doctest

comment:41 Changed 9 years ago by Punarbasu Purkayastha

Status: needs_reviewpositive_review

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

comment:42 Changed 9 years ago by Nathann Cohen

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

Nathann

comment:43 Changed 9 years ago by Ralf Stephan

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

Last edited 9 years ago by Nathann Cohen (previous) (diff)

comment:44 Changed 9 years ago by Volker Braun

Branch: u/ncohen/154829a7657bb5ce501543b7359e7bf19764d2b11e3cc
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.