Opened 4 years ago
Closed 3 years ago
#18763 closed enhancement (fixed)
COIN backend should support basis status and tableau data functions
Reported by:  mkoeppe  Owned by:  

Priority:  major  Milestone:  sage6.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 )
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.coinor.org/Osi/browser/trunk/Osi/src/OsiClp/OsiClpSolverInterface.hpp?rev=1472
Change History (16)
comment:1 Changed 4 years ago by
 Description modified (diff)
comment:2 Changed 3 years ago by
 Branch set to u/yzh/coin_backend_should_support_basis_status_and_tableau_data_functions
comment:3 Changed 3 years ago by
 Commit set to 9fdbd841cbd1585bca4633865e28440ad2ee5df0
comment:4 Changed 3 years ago by
 Commit changed from 9fdbd841cbd1585bca4633865e28440ad2ee5df0 to 3644950e0be45eb124c55931b57098415bbe7b21
Branch pushed to git repo; I updated commit sha1. New commits:
3644950  Add CoinBackend::get_binva_row, get_binva_col

comment:5 Changed 3 years ago by
 Status changed from new to needs_review
comment:6 followup: ↓ 7 Changed 3 years ago by
 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 ; followup: ↓ 9 Changed 3 years ago by
Instead of
sage_malloc
you could usecheck_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 3 years ago by
comment:9 in reply to: ↑ 7 ; followup: ↓ 11 Changed 3 years ago by
Hi Yuan,
Replying to yzh:
Instead of
sage_malloc
you could usecheck_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 bycheck_malloc
? Sincesage_malloc
is used throughout the file, I was trying to be consistent. Would you suggest replacing them all bycheck_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 3 years ago by
 Commit changed from 3644950e0be45eb124c55931b57098415bbe7b21 to 3ab6b33cab8eee152a417f4db45d5fe619fc797a
Branch pushed to git repo; I updated commit sha1. New commits:
3ab6b33  replace sage_malloc by check_malloc

comment:11 in reply to: ↑ 9 Changed 3 years ago by
 Status changed from needs_work to needs_review
Thank you, Vincent.
I changed sage_malloc
to check_malloc
.
comment:12 Changed 3 years ago by
 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 3 years ago by
 Commit changed from 3ab6b33cab8eee152a417f4db45d5fe619fc797a to c2488a8cf075664de211996ab3360ee8f4d48e56
Branch pushed to git repo; I updated commit sha1. New commits:
c2488a8  specify optionalcbc in doctests

comment:14 Changed 3 years ago by
 Status changed from needs_work to needs_review
Oops, I forgot to indicate optional  cbc at some places
comment:15 Changed 3 years ago by
 Milestone changed from sage6.8 to sage6.9
 Status changed from needs_review to positive_review
Good to go!
comment:16 Changed 3 years ago by
 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
Branch pushed to git repo; I updated commit sha1. New commits:
Fix bugs in CoinBackend::get/set_basis_status(); Add doctests