Opened 5 years ago

Closed 5 years ago

#15414 closed defect (fixed)

Cleanup in numerical.knapsack

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.1
Component: numerical Keywords:
Cc: defeo Merged in:
Authors: Nathann Cohen Reviewers: Luca De Feo
Report Upstream: N/A Work issues:
Branch: u/ncohen/15414 (Commits) Commit: 24c57700e67e74a7959a9585cbfe590bb1a6bde1
Dependencies: #15403 Stopgaps:

Description

This patch removes a copy of a paragraph that appears twice in numerical.knapsack, adds some backticks from place to place and solver/verbose optional arguments to the knapsack function.

Change History (12)

comment:1 Changed 5 years ago by ncohen

  • Branch set to u/ncohen/15414
  • Component changed from PLEASE CHANGE to numerical
  • Status changed from new to needs_review

comment:2 Changed 5 years ago by git

  • Commit set to da8b659b96e04b17842b994263694a667234db83

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

da8b659Cleanup in numerical.knapsack
4535875Improved knapsack's docstring -- review
23623ceImproved knapsack's docstring

comment:3 Changed 5 years ago by defeo

Hmm... "usefulnesss" is certainly wrong. I'd suggest "usefulnesses", but of course this is not in the dictionary. Any native speaker to the rescue?

comment:4 Changed 5 years ago by defeo

Building the docs, I get

[numerical] /home/dfl/sage/local/lib/python2.7/site-packages/sage/numerical/knapsack.py:
docstring of  sage.numerical.knapsack:34: WARNING: Bullet list ends without a blank line;
unexpected unindent.

But I really don't understand what it is complaining about. The docstring looks good to me around line 34.

comment:5 Changed 5 years ago by defeo

  • Reviewers set to Luca De Feo
  • Status changed from needs_review to needs_work

sage.numerical.mip.MixedIntegerLinearProgram.solve says the parameter solver is deprecated. It'd be better to only leave the link to sage.numerical.mip.MixedIntegerLinearProgram, where the solver parameter to the class is documented.

comment:6 Changed 5 years ago by defeo

Ok, last one. Shouldn't the default value for verbose be None? This looks more coherent with the doc of solve.

comment:7 Changed 5 years ago by ncohen

Allllllllll right ! I fixed them all by updating my commit. Except for your comment about "verbose". This variable has levels which can be integers, so 0 makes sense :-)

Nathann

comment:8 Changed 5 years ago by git

  • Commit changed from da8b659b96e04b17842b994263694a667234db83 to 24c57700e67e74a7959a9585cbfe590bb1a6bde1

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

24c5770Cleanup in numerical.knapsack
4535875Improved knapsack's docstring -- review
23623ceImproved knapsack's docstring

comment:9 Changed 5 years ago by defeo

  • Milestone changed from sage-5.13 to sage-6.0
  • Status changed from needs_work to positive_review

You rewrote history! I saw it, you did it! (not that I really care :)

I don't know how you fixed the warning, but you sure did! I have no more objections, let it go!

comment:10 Changed 5 years ago by ncohen

Yooooooo !!

There was ONE space missing at the beginning of the line

to 0 by default, which means quiet.

And now that history is rewritten, you may know it but you will never be able to prove it. And soon you will forget....

AHAHAHAHAHHAHAHAHAAAAAAAAAAAAAAAAHAHAHAHAHAHAAAAAAAAAAAAAA !!!

Nathann

comment:11 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.0 to sage-6.1

comment:12 Changed 5 years ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.