Opened 7 years ago
Closed 7 years ago
#19687 closed enhancement (fixed)
upgrade cvxopt to version 1.1.8
Reported by: | dimpase | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-7.0 |
Component: | packages: standard | Keywords: | |
Cc: | jdemeyer, ncohen | Merged in: | |
Authors: | Dima Pasechnik | Reviewers: | François Bissey |
Report Upstream: | Fixed upstream, but not in a stable release. | Work issues: | |
Branch: | 79784f3 (Commits, GitHub, GitLab) | Commit: | 79784f35dbb0752f160492dd574813e7e527f155 |
Dependencies: | Stopgaps: |
Description (last modified by )
this is the current stable version
The tarball is here: http://users.ox.ac.uk/~coml0531/sage/cvxopt-1.1.8.tar.gz
I removed html docs from the source (https://github.com/cvxopt/cvxopt/archive/1.1.8.tar.gz) to make it smaller.
Change History (26)
comment:1 Changed 7 years ago by
Branch: | → u/dimpase/19687 |
---|---|
Cc: | jdemeyer ncohen added |
Commit: | → c0405346c6d4d57f0c57e9822f20198fc2ccbfc5 |
Description: | modified (diff) |
Status: | new → needs_review |
comment:2 Changed 7 years ago by
Reviewers: | → François Bissey |
---|---|
Status: | needs_review → positive_review |
Looks good to me. No doctest broken on linux x86_64.
comment:3 follow-up: 6 Changed 7 years ago by
On older linux (Ubuntu 12.04) I get
ImportError: /mnt/highperf/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/cvxopt/amd.so: undefined symbol: clock_gettime
according to man page, needs to "link with -lrt (only for glibc versions before 2.17)" but does not.
comment:4 Changed 7 years ago by
Status: | positive_review → needs_work |
---|
comment:5 Changed 7 years ago by
Upstream probably doesn't build on older machines. Should be trivial to fix.
comment:6 Changed 7 years ago by
Replying to vbraun:
On older linux (Ubuntu 12.04) I get
ImportError: /mnt/highperf/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/cvxopt/amd.so: undefined symbol: clock_gettimeaccording to man page, needs to "link with -lrt (only for glibc versions before 2.17)" but does not.
It does not seem that testing glibc version tells you the right thing. Indeed, on Ubuntu 14.04, where this all works, I see
>>> import platform >>> platform.libc_ver() ('glibc', '2.4')
And I don't even have librt. I will just add it unconditionally, for every Linux...
comment:7 Changed 7 years ago by
Commit: | c0405346c6d4d57f0c57e9822f20198fc2ccbfc5 → 6390e86c33b9fdd7c47ee15592277ac7432926bb |
---|
comment:8 Changed 7 years ago by
Milestone: | sage-6.10 → sage-7.0 |
---|---|
Report Upstream: | N/A → Not yet reported upstream; Will do shortly. |
Status: | needs_work → needs_review |
comment:9 Changed 7 years ago by
Wrong solution
if SUITESPARSE_EXT_LIB: amd = Extension('amd', libraries = ['amd','suitesparseconfig'], include_dirs = [SUITESPARSE_INC_DIR], library_dirs = [SUITESPARSE_LIB_DIR], sources = ['src/C/amd.c']) else: amd = Extension('amd', include_dirs = [ 'src/C/SuiteSparse/AMD/Include', 'src/C/SuiteSparse/SuiteSparse_config' ], define_macros = MACROS, sources = [ 'src/C/amd.c', 'src/C/SuiteSparse/SuiteSparse_config/SuiteSparse_config.c'] + glob('src/C/SuiteSparse/AMD/Source/*.c') )
The amd
extension doesn't use libraries
so I would be surprised if it worked by adding it to BLAS_LIBS
.
comment:10 Changed 7 years ago by
Commit: | 6390e86c33b9fdd7c47ee15592277ac7432926bb → fdaeefa041f7c55ab914afc13ce9dcbe2859c2b2 |
---|
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
fdaeefa | add libs='rt' to amd ext.
|
comment:11 Changed 7 years ago by
right, it should have gone to the amd Extension chunk, sorry.
The patch is longer, as I fixed all the fizz, by cloning https://github.com/cvxopt/cvxopt
and making a new patch using the old one.
Then I just added libraries=['rt']
unconditionally there -- should this be done for Linux only?
comment:12 Changed 7 years ago by
No librt
on OS X as far as I can tell, I don't know about cygwin. I would restrict it.
comment:13 follow-up: 15 Changed 7 years ago by
clock_gettime
is not in the AMD
code it is in the SuiteSparse_config
I am checking the implications.
comment:14 Changed 7 years ago by
Commit: | fdaeefa041f7c55ab914afc13ce9dcbe2859c2b2 → 4b91b98e94b44d0559c97500eff70747cdf15271 |
---|
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
4b91b98 | add libs='rt' to amd ext (on linux only)
|
comment:15 Changed 7 years ago by
Replying to fbissey:
clock_gettime
is not in theAMD
code it is in theSuiteSparse_config
I am checking the implications.
well, the import error is in amd.so
, and src/C/SuiteSparse/SuiteSparse_config/SuiteSparse_config.c
gets complied into the amd extension. So this looks right what I do now...
comment:16 Changed 7 years ago by
Commit: | 4b91b98e94b44d0559c97500eff70747cdf15271 → e8c1e0a82f06364975b9b9f91c69de2443c9ad10 |
---|
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
e8c1e0a | add libs='rt' to amd ext (on linux only)
|
comment:17 Changed 7 years ago by
Volker reported one failure, there may be other hidden behind the first one. That file is also included in cholmod
and umfpack
so they are potentially affected too. However the code should only be enabled if SUITESPARSE_TIMER_ENABLED
is defined, in turn it depends on whether NTIMER
is defined (which it is for cholmod
and umfpack
so in the end they won't be affected). A better solution in my opinion is to add [('NTIMER', '1')]
to the line define_macros
for the amd
section.
New commits:
e8c1e0a | add libs='rt' to amd ext (on linux only)
|
comment:18 Changed 7 years ago by
adding things like [('NTIMER', '1')]
looks like going too far, as you're changing the behaviour of upstream code this way...
comment:19 Changed 7 years ago by
I don't think cvxopt
cares about timing of the code, in fact SuiteSparse_{tic,toc,time}
are used nowhere in AMD
's code. It is only enabled as part of SuiteSparse_config.c
by accident I would argue. In contrast cholmod
and umfpack
have code that can use SuiteSparse_time
and it is disabled on purpose in those. Those function are only used to measure performance, I don't think cvxopt
would be doing it that way, if it was interested in measuring the performance of the SuiteSparse
code.
comment:20 Changed 7 years ago by
I would further argue that we should point to upstream that unless they want to use those timing functions in the future, NTIMER
should be in the default macros variable that is currently empty.
comment:21 Changed 7 years ago by
Report Upstream: | Not yet reported upstream; Will do shortly. → Reported upstream. No feedback yet. |
---|
I just asked upstream here on this.
comment:22 Changed 7 years ago by
OK, I'll add an argument:
SuiteSparse_config.c
is not part of amd
's sources. If you were using an external suitesparse
install, you would have a suitesparse_config
library and amd
would get the functions from this library as needed. But here we have a custom system that makes SuiteSparse_config.c
a part of the used amd
source code and including all the function of SuiteSparse_config.c
instead of just the one needed by amd
. That include the timing functions.
comment:23 Changed 7 years ago by
Commit: | e8c1e0a82f06364975b9b9f91c69de2443c9ad10 → 79784f35dbb0752f160492dd574813e7e527f155 |
---|
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
79784f3 | fix building amd ext
|
comment:24 Changed 7 years ago by
Report Upstream: | Reported upstream. No feedback yet. → Fixed upstream, but not in a stable release. |
---|
comment:25 Changed 7 years ago by
Status: | needs_review → positive_review |
---|
I am happy with that resolution. Back to positive review.
comment:26 Changed 7 years ago by
Branch: | u/dimpase/19687 → 79784f35dbb0752f160492dd574813e7e527f155 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
New commits:
update to 1.1.8