Opened 4 years ago

Closed 15 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: sage-8.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 mkoeppe)

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 15 months ago by mkoeppe

  • Branch set to u/mkoeppe/mixedintegerlinearprogram_new_variable_could_optionally_take_a__static__list_of_component_indices

comment:2 Changed 15 months ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Cc tmonteil dcoudert tscrim added
  • Commit set to 7cb2b3a54149fa30ea6bc2f47b50a311dca6935d
  • Description modified (diff)
  • Milestone changed from sage-7.3 to sage-8.4
  • Status changed from new to needs_review

Needs review. Comments on the proposed interface are welcome.


New commits:

7cb2b3aMIPVariable, MixedIntegerLinearProgram.new_variable: Take optional static list of component indices

comment:3 follow-up: Changed 15 months ago by tscrim

  • 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 ; follow-ups: Changed 15 months ago by 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 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 static-index MIP variable for which any component access is an error.)

comment:5 Changed 15 months ago by mkoeppe

  • Description modified (diff)

comment:6 in reply to: ↑ 4 Changed 15 months ago by tscrim

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 on Polyhedron I remember that one developer (Volker?) seems to hold the opinion that None 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 non-False-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 15 months ago by git

  • Commit changed from 7cb2b3a54149fa30ea6bc2f47b50a311dca6935d to 604b2c76f28df5d7b9684a2c874ee41e9de51bfd

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

604b2c7MIPVariable, MixedIntegerLinearProgram.new_variable: Use None instead of 'dynamic'

comment:8 Changed 15 months ago by mkoeppe

Thanks for the discussion. I've changed it to None. Needs review

comment:9 Changed 15 months ago by git

  • Commit changed from 604b2c76f28df5d7b9684a2c874ee41e9de51bfd to 23689ddb190d6e2847fd492c91c996a7afa2bbea

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

23689ddMIPVariable.__getitem__: Add doctest

comment:10 in reply to: ↑ 4 Changed 15 months ago by mkoeppe

Replying to mkoeppe:

I definitely want to distinguish [] from the default argument ...

(indices = [] must give a static-index MIP variable for which any component access is an error.)

I've added a doctest for this.

Ready for final review

comment:11 Changed 15 months ago by tscrim

  • Status changed from needs_review to positive_review

Thanks. LGTM.

comment:12 Changed 15 months ago by vbraun

  • 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
Note: See TracTickets for help on using tickets.