Opened 6 years ago

Closed 5 years ago

#15482 closed defect (fixed)

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

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

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 6 years ago by ncohen

  • Branch set to u/ncohen/15482
  • Status changed from new to needs_review

comment:2 Changed 6 years ago by git

  • Commit set to 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 6 years ago by ppurka

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 6 years ago by ncohen

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 6 years ago by git

  • Commit changed from 1f6c156dfd70534f26ddb2e639f57c040dc199fa to b50d7af25a94022efa29eccd3637bcd9396fab16

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

b50d7aftrac #15482: addressing the reviewer's comments

comment:6 Changed 6 years ago by ppurka

  • Reviewers set to Punarbasu Purkayastha
  • Status changed from needs_review to positive_review

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

comment:7 Changed 6 years ago by kcrisman

  • Status changed from positive_review to needs_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 6 years ago by ppurka

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 6 years ago by ncohen

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 6 years ago by git

  • Commit changed from b50d7af25a94022efa29eccd3637bcd9396fab16 to 9a666314188dc3681dea63922458096e6380ea41

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 6 years ago by ncohen

  • Status changed from needs_work to needs_review

comment:12 Changed 6 years ago by kcrisman

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 6 years ago by ncohen

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 6 years ago by ncohen

(just created #15489 for this dim problem)

Nathann

comment:15 Changed 6 years ago by ppurka

  • Reviewers changed from Punarbasu Purkayastha to Punarbasu Purkayastha, Karl-Dieter Crisman
  • Status changed from needs_review to positive_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 6 years ago by git

  • Commit changed from 9a666314188dc3681dea63922458096e6380ea41 to d2870e1a5a9d449ecabf1859f2d380308eb5dcab
  • 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:

d2870e1trac #15482: shortening a line

comment:17 Changed 6 years ago by ncohen

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

Nathann

comment:18 Changed 6 years ago by ncohen

  • Status changed from needs_review to positive_review

comment:19 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.13 to sage-6.0

comment:20 Changed 6 years ago by tmonteil

  • Status changed from positive_review to needs_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 6 years ago by git

  • Commit changed from d2870e1a5a9d449ecabf1859f2d380308eb5dcab to cd5bb43f35adfc8c03c2893275aa8245d76b1204

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

cd5bb43trac #15482: positive -> non-negative

comment:22 Changed 6 years ago by ncohen

  • Status changed from needs_info to positive_review

Done.

comment:23 follow-up: Changed 6 years ago by tmonteil

  • Reviewers changed from Punarbasu Purkayastha, Karl-Dieter Crisman to Punarbasu Purkayastha, Karl-Dieter Crisman, Thierry Monteil
  • Summary changed from Say very loud that LP variables are positive by default to Say very loud that LP variables are non-negative by default

non-negative review :P

comment:24 in reply to: ↑ 23 Changed 6 years ago by kcrisman

non-negative review :P

LOL

comment:25 Changed 6 years ago by git

  • Commit changed from cd5bb43f35adfc8c03c2893275aa8245d76b1204 to dd7bde803644daee20e4b0439914814eb1cf48cb
  • 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:

dd7bde8trac #15482: a broken doctest

comment:26 Changed 6 years ago by ncohen

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 6 years ago by git

  • Commit changed from dd7bde803644daee20e4b0439914814eb1cf48cb to c8addd909e463c56949babef9804f603742381dc

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

c8addd9trac #15482: rebase on 5.13.rc0

comment:28 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.0 to sage-6.1

comment:29 Changed 5 years ago by ppurka

What is up with the hundreds of failing doctests? o_O

comment:30 Changed 5 years ago by ncohen

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 5 years ago by ncohen

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 5 years ago by ncohen

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

comment:33 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:34 Changed 5 years ago by git

  • Commit changed from c8addd909e463c56949babef9804f603742381dc to 076f85bd75e7f69706bf4bd238a8a50f8d180bc0

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

076f85btrac #15482: Rebase on 6.2.beta0

comment:35 Changed 5 years ago by rws

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

comment:36 Changed 5 years ago by git

  • Commit changed from 076f85bd75e7f69706bf4bd238a8a50f8d180bc0 to b87f1deb520451cc8d770c9b3e4f9268070912b8

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

b87f1detrac #15482: Rebase on 6.2.beta1

comment:37 Changed 5 years ago by ncohen

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 5 years ago by ppurka

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 5 years ago by ncohen

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 5 years ago by git

  • Commit changed from b87f1deb520451cc8d770c9b3e4f9268070912b8 to 9a7657bb5ce501543b7359e7bf19764d2b11e3cc

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

9a7657btrac #15482: Broken doctest

comment:41 Changed 5 years ago by ppurka

  • Status changed from needs_review to positive_review

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

comment:42 Changed 5 years ago by ncohen

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

Nathann

comment:43 Changed 5 years ago by rws

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

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

comment:44 Changed 5 years ago by vbraun

  • Branch changed from u/ncohen/15482 to 9a7657bb5ce501543b7359e7bf19764d2b11e3cc
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.