Opened 3 years ago

Closed 3 years ago

#20304 closed defect (fixed)

More error checking in MixedIntegerLinearProgram

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-7.2
Component: numerical Keywords: lp
Cc: dimpase, ncohen Merged in:
Authors: Matthias Koeppe Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: 7d7aeed (Commits) Commit: 7d7aeed0d1ccfbb1651f8b296f9913b0db5d1b6b
Dependencies: Stopgaps:

Description (last modified by mkoeppe)

The get_values method silently ignored non-variables.

Change History (16)

comment:1 Changed 3 years ago by mkoeppe

  • Branch set to u/mkoeppe/more_error_checking_in_mixedintegerlinearprogram

comment:2 Changed 3 years ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Cc dimpase ncohen added
  • Commit set to dc3895bb02227dff5b2f7130e43f877754c4861b
  • Component changed from PLEASE CHANGE to numerical
  • Keywords lp added
  • Status changed from new to needs_review
  • Type changed from PLEASE CHANGE to defect

The get_values method silently ignored non-variables.


New commits:

dc3895bMixedIntegerLinearProgram.get_values: Input checking

comment:3 Changed 3 years ago by mkoeppe

  • Description modified (diff)

comment:4 Changed 3 years ago by vdelecroix

  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to needs_work

Please use new style error ValueError(msg) instead of ValueError, msg (Python 3 compatibility).

Would be better to use repr rather than str. The output of "{!r}".format(l) is nicer than str(l). As you can see

sage: l = 1
sage: str(l)
'1'
sage: "{!r}".format(l)
'1'
sage: l = '1'
sage: str(l)
'1'
sage: "{!r}".format(l)
"'1'"

The following are not caught correctly

sage: M1 = MixedIntegerLinearProgram()
sage: M2 = MixedIntegerLinearProgram()
sage: x = M1.new_variable()
sage: y = M1.new_variable()
sage: z = M2.new_variable()
sage: M2.add_constraint(z[0] <= 5)
sage: M2.solve()
0.0
sage: M2.get_values(x)              # should be a ValueError
Traceback (most recent call last):
...
KeyError: x_0
sage: M2.get_values(y)              # should be a ValueError
{}

comment:5 Changed 3 years ago by vdelecroix

And if the input is not a variable it would be better to raise a TypeError rather than a ValueError.

comment:6 Changed 3 years ago by git

  • Commit changed from dc3895bb02227dff5b2f7130e43f877754c4861b to 4290bde5ae4b40b8ae434fce93db624c4c76d873

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

4290bdeMixedIntegerLinearProgram: use new style error ValueError(msg) instead of ValueError, msg (Python 3 compatibility).

comment:7 Changed 3 years ago by git

  • Commit changed from 4290bde5ae4b40b8ae434fce93db624c4c76d873 to 13e82d9944fbf5f64c6ea6559a7c6be221aaea09

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

67a263dMIPVariable: New method mip
13e82d9MixedIntegerLinearProgram.get_values: Also check that a MIPVariable is from the right MIP

comment:8 Changed 3 years ago by mkoeppe

  • Status changed from needs_work to needs_review

comment:9 Changed 3 years ago by vdelecroix

Nice.

Last thing: the first error should be a ValueError. The type is indeed the good one!

comment:10 follow-up: Changed 3 years ago by mkoeppe

Should I then make it a ValueError also when it's a string, but not one that names a variable? (If so, what's the right way to test if it is a string?)

comment:11 in reply to: ↑ 10 Changed 3 years ago by vdelecroix

Replying to mkoeppe:

Should I then make it a ValueError also when it's a string, but not one that names a variable? (If so, what's the right way to test if it is a string?)

Not necessarily (one can assume that the "good" way of calling this function is with a MIP variable). Though, to test if the object x is a string you can use isinstance(x, str) (which returns a boolean).

comment:12 Changed 3 years ago by mkoeppe

I was confused, it actually does not accept strings -- only MIPVariable objects.

Last edited 3 years ago by mkoeppe (previous) (diff)

comment:13 Changed 3 years ago by git

  • Commit changed from 13e82d9944fbf5f64c6ea6559a7c6be221aaea09 to 7d7aeed0d1ccfbb1651f8b296f9913b0db5d1b6b

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

7d7aeedMixedIntegerLinearProgram.get_values: Raise TypeError when not a MIPVariable, ValueError when from the wrong MIP

comment:14 Changed 3 years ago by vdelecroix

  • Status changed from needs_review to positive_review

comment:15 Changed 3 years ago by mkoeppe

Thanks for reviewing!

comment:16 Changed 3 years ago by vbraun

  • Branch changed from u/mkoeppe/more_error_checking_in_mixedintegerlinearprogram to 7d7aeed0d1ccfbb1651f8b296f9913b0db5d1b6b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.