Opened 3 years ago
Closed 10 months ago
#20773 closed enhancement (fixed)
MixedIntegerLinearProgram.new_variable could optionally take a "static" list of component indices
Reported by:  mkoeppe  Owned by:  

Priority:  major  Milestone:  sage8.4 
Component:  numerical  Keywords:  lp 
Cc:  dimpase, vdelecroix, jdemeyer, nbruin, chapoton, tmonteil, dcoudert, tscrim  Merged in:  
Authors:  Matthias Koeppe  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  23689dd (Commits)  Commit:  23689ddb190d6e2847fd492c91c996a7afa2bbea 
Dependencies:  Stopgaps: 
Description (last modified by )
Currently, new_variable
allocates backend indices to MIP variable component indices very dynamically.
sage: p = MixedIntegerLinearProgram() sage: x = p.new_variable() sage: x[7] x_0 sage: x[2] x_1
Sometimes it could be nice to have a predictable order in the backend; for example, when .polyhedron()
is to be called. And sometimes it could be nice to declare ahead of time which component indices are valid, making accesses to other indices an error (rather than creating a new backend variable); this could help debug MIP models.
This could look like this:
sage: p = MixedIntegerLinearProgram() sage: x = p.new_variable(indices=range(7)) sage: p.number_of_variables() 7 sage: x[3] x_3 sage: x[11] IndexError: 11 does not index a component of MIPVariable of dimension 1
(This is optional; if no indices
argument is passed, it keeps the fully dynamic current behavior.)
Change History (12)
comment:1 Changed 10 months ago by
 Branch set to u/mkoeppe/mixedintegerlinearprogram_new_variable_could_optionally_take_a__static__list_of_component_indices
comment:2 Changed 10 months ago by
 Cc tmonteil dcoudert tscrim added
 Commit set to 7cb2b3a54149fa30ea6bc2f47b50a311dca6935d
 Description modified (diff)
 Milestone changed from sage7.3 to sage8.4
 Status changed from new to needs_review
comment:3 followup: ↓ 4 Changed 10 months ago by
 Reviewers set to Travis Scrimshaw
While it might be a little less verbose, it is more conceptually an optional argument if the default was None
. Then you could also write the doc as
 ``indices``  (optional) an iterable of keys; components corresponding to these keys are created in order, and access to components with other keys will raise an error; otherwise components of this variable can be indexed by arbitrary keys and are created dynamically on access
However, I don't care strongly either way. So if you want to keep the current version, then you can set a positive review on my behalf.
comment:4 in reply to: ↑ 3 ; followups: ↓ 6 ↓ 10 Changed 10 months ago by
Replying to tscrim:
While it might be a little less verbose, it is more conceptually an optional argument if the default was
None
.
Right, but I definitely want to distinguish []
from the default argument (and from a discussion on Polyhedron
I remember that one developer (Volker?) seems to hold the opinion that None
and []
should never (?) be distinguished by Python methods).
(indices = []
must give a staticindex MIP variable for which any component access is an error.)
comment:5 Changed 10 months ago by
 Description modified (diff)
comment:6 in reply to: ↑ 4 Changed 10 months ago by
Replying to mkoeppe:
Replying to tscrim:
While it might be a little less verbose, it is more conceptually an optional argument if the default was
None
.Right, but I definitely want to distinguish
[]
from the default argument (and from a discussion onPolyhedron
I remember that one developer (Volker?) seems to hold the opinion thatNone
and[]
should never (?) be distinguished by Python methods).
That is not a good rule IMO:
sage: Partition([]) [] sage: Partition(None) # This makes no sense and rightly raises an error.
The only thing you have to be careful about is if foo:
just needs to be if foo is None:
. It seems like you having such a nonFalse
bool default value is more likely to cause issues given how Python code usually can be written, and it can mean a more arbitrary default that has less meaning for the code. It also does not mesh well with Cython either. I would be curious to hearing what the argument for this is, but I think None
and []
should be considered as distinguishable. Unless you also want False
, ()
, and 0
to not be distinguishable either? At least, that is what I think the logic of this rule leads me to conclude.
comment:7 Changed 10 months ago by
 Commit changed from 7cb2b3a54149fa30ea6bc2f47b50a311dca6935d to 604b2c76f28df5d7b9684a2c874ee41e9de51bfd
Branch pushed to git repo; I updated commit sha1. New commits:
604b2c7  MIPVariable, MixedIntegerLinearProgram.new_variable: Use None instead of 'dynamic'

comment:8 Changed 10 months ago by
Thanks for the discussion. I've changed it to None
. Needs review
comment:9 Changed 10 months ago by
 Commit changed from 604b2c76f28df5d7b9684a2c874ee41e9de51bfd to 23689ddb190d6e2847fd492c91c996a7afa2bbea
Branch pushed to git repo; I updated commit sha1. New commits:
23689dd  MIPVariable.__getitem__: Add doctest

comment:10 in reply to: ↑ 4 Changed 10 months ago by
Replying to mkoeppe:
I definitely want to distinguish
[]
from the default argument ...(
indices = []
must give a staticindex MIP variable for which any component access is an error.)
I've added a doctest for this.
Ready for final review
comment:11 Changed 10 months ago by
 Status changed from needs_review to positive_review
Thanks. LGTM.
comment:12 Changed 10 months ago by
 Branch changed from u/mkoeppe/mixedintegerlinearprogram_new_variable_could_optionally_take_a__static__list_of_component_indices to 23689ddb190d6e2847fd492c91c996a7afa2bbea
 Resolution set to fixed
 Status changed from positive_review to closed
Needs review. Comments on the proposed interface are welcome.
New commits:
MIPVariable, MixedIntegerLinearProgram.new_variable: Take optional static list of component indices