search

devernay / cminpack

A C/C++ rewrite of the MINPACK software (originally in FORTRAN) for solving nonlinear equations and nonlinear least squares problems
http://devernay.free.fr/hacks/cminpack/
145 stars 63 forks source link

Disable BLAS by default #66

Closed jschueller closed 2 months ago

jschueller commented 3 months ago

Enabling BLAS makes cminpack crash because integer arguments are passed by value instead of reference:

https://github.com/devernay/cminpack/blob/master/cminpackP.h#L18

devernay commented 2 months ago

Can you please elaborate? As documented, this flag is for using cblas, which has C calling conventions, and function names not ending with an underscore. Integers are passed by value in cblas. Which version of cblas are you using?

devernay commented 2 months ago

Oh I see now... We switched from cblas to blas 3 years ago, but the function signatures were not changed. Can you please make a PR to update cminpackP.h instead?

devernay commented 2 months ago

See #53

devernay commented 2 months ago

https://github.com/devernay/cminpack/pull/68 tries to really fix the issue. Does that look ok to you?

jschueller commented 2 months ago

I got this error with your branch:

/cminpack/enorm_.c:59:11: error: unknown type name ‘__cminpack_blasint__’
   59 |     const __cminpack_blasint__ c__1 = 1;

this should probably be tested in the CI

devernay commented 2 months ago

Yes, it's still a draft

devernay commented 2 months ago

updated #68

jschueller commented 2 months ago

ok, it builds now

jschueller commented 2 months ago

configuration fails if blas is unavailable (ie windows) or if extended precision is enabled (by default) should we still disable blas by default, or use it only if available and only for non extended precision variants ?

devernay commented 2 months ago

I think BLAS support should be disabled by default, because results may depend on the BLAS implementation, and this may lead to unexpected results, depending on which BLAS is installed.

chenrui333 commented 1 week ago

I think BLAS support should be disabled by default, because results may depend on the BLAS implementation, and this may lead to unexpected results, depending on which BLAS is installed.

is openblas supported, if so, can we maybe add it to the CI?

relates to https://github.com/Homebrew/homebrew-core/pull/183713