Opened 10 years ago

Closed 9 years ago

#7789 closed enhancement (fixed)

Improve the arguments for the default type of a variable in MixedIntegerLinearProgram

Reported by: ncohen Owned by: jkantor
Priority: major Milestone: sage-4.4.4
Component: numerical Keywords:
Cc: Merged in: sage-4.4.4.alpha0
Authors: Nathann Cohen Reviewers: Joni Syri
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by malb)

The help of MixedIntegerLinearProgram.new_variable shows a way to define a default type for new variables, but it uses the argument vtype and pre-defined constants (MixedIntegerLinearProgram.__INTEGER for example) which is really ugly.

We should accept things like :

p.new_variable(boolean=True)

or

p.new_variable(type="boolean")

Attachments (1)

trac_7789.patch (3.0 KB) - added by ncohen 9 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 10 years ago by ncohen

  • Status changed from new to needs_review

Here it is !

comment:2 Changed 9 years ago by jason

  • Type changed from defect to enhancement

comment:3 Changed 9 years ago by wdj

  • Status changed from needs_review to needs_work

I think this needs more examples.

comment:4 Changed 9 years ago by ncohen

  • Status changed from needs_work to needs_review

I just updated the patch to add 2 lines of test .. I am sorry but I do not really know which kind of examples you expect there... :-/

Please tell me and I'll add them immediately !!

Nathann

comment:5 Changed 9 years ago by malb

  • Description modified (diff)

comment:6 Changed 9 years ago by malb

You current code allows real=True and binary=True and will quietly make a choice.

comment:7 Changed 9 years ago by ncohen

Fixed !

comment:8 Changed 9 years ago by jsyri

  • Status changed from needs_review to needs_work

Few comments:

I think parameter 'name' should be documented. I couldn't find out any place where changing this parameter would affect at all. Also show() method lists 'binary' type variables as boolean variables, which I find bit ugly. Fixing that might be out of scope of this ticket though, as 'binary' is used extensively in code and documentation.

I'm not native English speaker so I might be wrong here, but I think this: "An exception is raised when two types are required" at the documentation should maybe say something like "An exception is raised when two types are _supplied_".

comment:9 Changed 9 years ago by ncohen

  • Status changed from needs_work to needs_review

Sounds comments... Updated ! :-)

Nathann

comment:10 Changed 9 years ago by jsyri

  • Status changed from needs_review to needs_work

Unfortunately indentation at documentation for 'name' is off by one, so one more update required :-( Otherwise everything seems fine. I'm running doctest now, and if they come clean it'll get positive review after that last fix.

comment:11 Changed 9 years ago by ncohen

  • Status changed from needs_work to needs_review

Changed 9 years ago by ncohen

comment:12 Changed 9 years ago by ncohen

Done.

comment:13 Changed 9 years ago by jsyri

  • Status changed from needs_review to positive_review

Everything seems to be in order. Positive review it is.

comment:14 Changed 9 years ago by mhansen

  • Authors set to Nathann Cohen
  • Merged in set to sage-4.4.4.alpha0
  • Resolution set to fixed
  • Reviewers set to Joni Syri
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.