Opened 4 years ago

Closed 4 years ago

#18763 closed enhancement (fixed)

COIN backend should support basis status and tableau data functions

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-6.9
Component: numerical Keywords: lp
Cc: yzh, ncohen Merged in:
Authors: Yuan Zhou Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: c2488a8 (Commits) Commit: c2488a8cf075664de211996ab3360ee8f4d48e56
Dependencies: Stopgaps:

Description (last modified by mkoeppe)

This is for COIN backend support in #18733 and #18688.

Should add (1) getBasisStatus, setBasisStatus, (2) getBInvARow, getBInvACol, getBasics, to src/sage/numerical/backends/coin_backend.{pxd,pyx}

Possible that one needs to add other functions as well, such as enableSimplexInterface...

Reference to the relevant Clp header file: https://projects.coin-or.org/Osi/browser/trunk/Osi/src/OsiClp/OsiClpSolverInterface.hpp?rev=1472

Change History (16)

comment:1 Changed 4 years ago by mkoeppe

  • Description modified (diff)

comment:2 Changed 4 years ago by yzh

  • Branch set to u/yzh/coin_backend_should_support_basis_status_and_tableau_data_functions

comment:3 Changed 4 years ago by git

  • Commit set to 9fdbd841cbd1585bca4633865e28440ad2ee5df0

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

9fdbd84Fix bugs in CoinBackend::get/set_basis_status(); Add doctests

comment:4 Changed 4 years ago by git

  • Commit changed from 9fdbd841cbd1585bca4633865e28440ad2ee5df0 to 3644950e0be45eb124c55931b57098415bbe7b21

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

3644950Add CoinBackend::get_binva_row, get_binva_col

comment:5 Changed 4 years ago by yzh

  • Status changed from new to needs_review

comment:6 follow-up: Changed 4 years ago by vdelecroix

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

Please fill the Authors field with your full name.

Instead of sage_malloc you could use check_malloc (that checks that the memory is indeed allocated and raises an error if not).

Otherwise, everything looks clean and tests run smoothly on my computer (with Coin installed).

Vincent

comment:7 in reply to: ↑ 6 ; follow-up: Changed 4 years ago by yzh

Instead of sage_malloc you could use check_malloc (that checks that the memory is indeed allocated and raises an error if not).

Thanks a lot for reviewing the ticket. Does one call sage_free to free the memory allocated by check_malloc? Since sage_malloc is used throughout the file, I was trying to be consistent. Would you suggest replacing them all by check_malloc? Or perhaps better to update them using #18868?

comment:8 Changed 4 years ago by yzh

  • Authors set to Yuan Zhou

comment:9 in reply to: ↑ 7 ; follow-up: Changed 4 years ago by vdelecroix

Hi Yuan,

Replying to yzh:

Instead of sage_malloc you could use check_malloc (that checks that the memory is indeed allocated and raises an error if not).

Thanks a lot for reviewing the ticket. Does one call sage_free to free the memory allocated by check_malloc? Since sage_malloc is used throughout the file, I was trying to be consistent. Would you suggest replacing them all by check_malloc? Or perhaps better to update them using #18868?

I do not think that #18868 is good idea in this case. You will create a Python object for nothing.

check_malloc is equivalent to sage_malloc except that it will raise an error in case the memory was not allocated (where sage_malloc just returns NULL). So the memory should be freed with sage_free. You can safely replace occurrences of sage_malloc by check_malloc. Moreover you can always replace

my_ptr = sage_malloc(12)
if my_ptr == NULL:
    raise MemoryError("allocation failed")

by

my_ptr = check_malloc(12)

Vincent

comment:10 Changed 4 years ago by git

  • Commit changed from 3644950e0be45eb124c55931b57098415bbe7b21 to 3ab6b33cab8eee152a417f4db45d5fe619fc797a

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

3ab6b33replace sage_malloc by check_malloc

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

  • Status changed from needs_work to needs_review

Thank you, Vincent. I changed sage_malloc to check_malloc.

comment:12 Changed 4 years ago by vdelecroix

  • Status changed from needs_review to needs_work

The patchbot reported some errors (when cbc is not installed)

sage -t --long src/sage/numerical/backends/coin_backend.pyx
**********************************************************************
File "src/sage/numerical/backends/coin_backend.pyx", line 1260, in sage.numerical.backends.coin_backend.CoinBackend.get_basis_status
Failed example:
    p.solve()
    ...
    NameError: name 'p' is not defined
**********************************************************************
File "src/sage/numerical/backends/coin_backend.pyx", line 1285, in sage.numerical.backends.coin_backend.CoinBackend.get_basis_status
Failed example:
    lp_coin = lp.get_backend()
    ...
    NameError: name 'lp' is not defined
**********************************************************************

comment:13 Changed 4 years ago by git

  • Commit changed from 3ab6b33cab8eee152a417f4db45d5fe619fc797a to c2488a8cf075664de211996ab3360ee8f4d48e56

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

c2488a8specify optional-cbc in doctests

comment:14 Changed 4 years ago by yzh

  • Status changed from needs_work to needs_review

Oops, I forgot to indicate optional - cbc at some places

comment:15 Changed 4 years ago by vdelecroix

  • Milestone changed from sage-6.8 to sage-6.9
  • Status changed from needs_review to positive_review

Good to go!

comment:16 Changed 4 years ago by vbraun

  • Branch changed from u/yzh/coin_backend_should_support_basis_status_and_tableau_data_functions to c2488a8cf075664de211996ab3360ee8f4d48e56
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.