Opened 4 years ago

Closed 4 years ago

#20326 closed defect (fixed)

GenericBackend: Fix doctest of add_linear_constraint_vector

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-7.2
Component: numerical Keywords: lp
Cc: dimpase, vdelecroix Merged in:
Authors: Matthias Koeppe Reviewers: Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 690b1a5 (Commits) Commit: 690b1a55435f70abfd17bfcc752af13a7cce8e52
Dependencies: Stopgaps:

Description (last modified by dimpase)

The doctest of add_linear_constraint_vector from generic_backend.pyx (which is never run for any real backend!), when applied to COIN or GLPK leads to segfaults:

sage: coeffs = ([0, vector([1, 2])], [1, vector([2, 3])])
sage: upper = vector([5, 5])
sage: lower = vector([0, 0])
sage: from sage.numerical.backends.generic_backend import get_solver
sage: p = get_solver(solver = "Coin")      # optional - cbc
sage: p.add_linear_constraint_vector(2, coeffs, lower, upper, 'foo')
------------------------------------------------------------------------
0   signals.so                          0x0000000109df05c5 print_backtrace + 37
------------------------------------------------------------------------
Unhandled SIGSEGV: A segmentation fault occurred.
This probably occurred because a *compiled* module has a bug
in it and is not properly wrapped with sig_on(), sig_off().
Python will now terminate.
------------------------------------------------------------------------
Segmentation fault: 11
$ sage
SageMath Version 7.2.beta0, Release Date: 2016-03-24 
sage: coeffs = ([0, vector([1, 2])], [1, vector([2, 3])])
sage: upper = vector([5, 5])
sage: lower = vector([0, 0])
sage: from sage.numerical.backends.generic_backend import get_solver
sage: p = get_solver(solver = "Coin")       # optional - cbc
sage: p.add_linear_constraint_vector(2, coeffs, lower, upper)
------------------------------------------------------------------------
0   signals.so                          0x0000000109c8a5c5 print_backtrace + 37
------------------------------------------------------------------------
Unhandled SIGSEGV: A segmentation fault occurred.

Change History (14)

comment:1 Changed 4 years ago by dimpase

  • Description modified (diff)

with the second example I only get segfault with Coin - with other backends I get gracefully handled errors. (on a branch with lots of recent positively reviewed LP-related tickets)

Last edited 4 years ago by dimpase (previous) (diff)

comment:2 Changed 4 years ago by mkoeppe

The COIN segfaults are addressed in #20360.

comment:3 Changed 4 years ago by mkoeppe

  • Branch set to u/mkoeppe/genericbackend__fix_doctest_of_add_linear_constraint_vector

comment:4 Changed 4 years ago by mkoeppe

  • Cc ncohen removed
  • Commit set to dea0705ca3ff339dc7887ae040c1c787d03798a0
  • Status changed from new to needs_review

New commits:

dea0705Fix #20326: GenericBackend: Fix doctest of add_linear_constraint_vector

comment:5 Changed 4 years ago by dimpase

solver.add_variables(2) throws a NotImplementedError immediately, what's the point of the last commit then?

sage -t src/sage/numerical/backends/generic_backend.pyx
**********************************************************************
File "src/sage/numerical/backends/generic_backend.pyx", line 402, in sage.numerical.backends.generic_backend.GenericBackend.add_linear_constraint_vector
Failed example:
    solver.add_variables(2)
...
    NotImplementedError

comment:6 Changed 4 years ago by dimpase

if you want to keep these as a template you'd mark them # not tested

comment:7 Changed 4 years ago by git

  • Commit changed from dea0705ca3ff339dc7887ae040c1c787d03798a0 to b932b890e06615af0d8501127e6dbcfb49c4dff3

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

b932b89add_linear_constraint_vector: Fix fix of doctest

comment:8 Changed 4 years ago by mkoeppe

Sorry! I tested the wrong file after making this change, so I did not catch this test failure.

Yes, the idea is to make generic_backend.pyx suitable for cut and paste again. But don't worry, I won't make many more tickets like this one; I have been making progress with #20323.

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

comment:9 follow-up: Changed 4 years ago by dimpase

Isn't it more in line with the rest of the code to tag these statements by # optional - Nonexistent_LP_solver ? Then you don't need to insert Tracebacks...

comment:10 Changed 4 years ago by git

  • Commit changed from b932b890e06615af0d8501127e6dbcfb49c4dff3 to 690b1a55435f70abfd17bfcc752af13a7cce8e52

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

b3a14bbFix #20326: GenericBackend: Fix doctest of add_linear_constraint_vector
690b1a5add_linear_constraint: Use 'optional - Nonexistent_LP_solver' test as well

comment:11 in reply to: ↑ 9 Changed 4 years ago by mkoeppe

Replying to dimpase:

Isn't it more in line with the rest of the code to tag these statements by # optional - Nonexistent_LP_solver ? Then you don't need to insert Tracebacks...

You are right. I have now brought it in line with the doctests for the other methods. Same also for add_linear_constraint.

comment:12 Changed 4 years ago by dimpase

  • Status changed from needs_review to positive_review

OK, good!

comment:13 Changed 4 years ago by dimpase

  • Authors set to Matthias Koeppe
  • Reviewers set to Dima Pasechnik

comment:14 Changed 4 years ago by vbraun

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