When using CMake, always also build single-precision variant.#45
When using CMake, always also build single-precision variant.#45devernay merged 1 commit intodevernay:masterfrom
Conversation
devernay
left a comment
There was a problem hiding this comment.
Thank you!
I'm OK with this change, but can you move the doc about building with cmake, and the fact that it builds both libraries, is a separate section of the doc as indicated below?
I would also like feedback from other contributors (@xantares @bherisse @tadeu @chaplin89): Is everyone OK with systematically building the 2 versions?
Also, why not the "long double" (128-bit float) version? The "half" version is really just a proof-of-concept, but "long double" may be useful if precision is an issue. long double is not supported by every compiler/platform so there would have to be a test for that. I'm not a CMake afficionado, so I don't know the proper way of testing this, but the FFTW configure script does this test.
docs/index.html
Outdated
|
|
||
| <p>The CMinpack calls have the same name as the FORTRAN functions, in lowercase (e.g. <code>lmder(...)</code>). See the links to the documentation below, or take a look at the simple examples in the <code>examples</code> directory of the distribution. The simple examples are named after the function they call: <code>tlmder.c</code> is the simple example for <code>lmder</code>.</p> | ||
| <p>If you want to use the single precision CMinpack, you should define __cminpack_float__ before including <code>cminpack.h</code>. __cminpack_half__ has to be defined for the half-precision version (and the code needs to be compiled with a C++ compiler).</p> | ||
| <p>If you want to use the single precision CMinpack, you should define __cminpack_float__ before including <code>cminpack.h</code>. __cminpack_half__ has to be defined for the half-precision version (and the code needs to be compiled with a C++ compiler). By default, building via CMake will generate both a double precision librarty, named <code>cminpack</code>, and a single precision library, named <code>cminpacks</code>.</p> |
There was a problem hiding this comment.
This section is about using, not building cminpack, so I don't think this comment should be here.
We should probably have a small "Building cminpack" section above that that gives the typical building instructions for cmake, and adds that information. Can you do that?
Those building instructions should also mention the Makefile-based build:
The source distribution also contains a Makefile which can be used to build cminpack for single-precision (make single), half-precision (make half), long double precision (make longdouble), and even CUDA (make cuda), or using LAPACK for linear algebra (make lapack). Unit tests are also provided, but results may depend on the platform (make targets checkdouble, checklapack ,checklongdouble, checkfloat, checkhalf).
There was a problem hiding this comment.
done
Edit: Actually I haven't been able to build the double precision variant on Windows: both on Appveyor and locally, I get the same error: "enorm.c(105): error C2177: constant too big"/"\enorm_.c(113): error C2177: constant too big". So for now I just removed the longdouble target from "all", but it can easily be added back if the error gets fixed.
|
Also, it's OK to reformat the CMakelist, but it's easier to review if it's a separate PR. |
|
Are you sure you need both variants at the same time ? What's the use-case ? |
The use case is when cminpack is packaged by redistributors, which include at least linux distros and conda. In that case they will typically just provide a "plain" build of the library, which here means only the double-precision version. I would rather have them provide all precisions together, so that I can easily have whatever precision I need (single, in my case) on machines with no compiler available.
Per the above I believe the default should be to provide all of them (because otherwise I'd need to go to each and every redistributor and convince them that they should do a non-default build of the library), but we could reasonably add a CMake flag to just build one of them (now implemented). |
d4302b4 to
e3c4260
Compare
|
can you rebase and push the changes? |
|
done |
|
@anntzer I'm curious, do you have an automatic formatter for CMakeLists or did you do it by hand? |
|
I did it by hand plus some regexes... |
|
Thanks for the offer of being a collaborator. I don't claim any expertise in the relevant field (I really just needed a reasonable implementation of |
|
@devernay Any chance for a new release with these changes? Thanks! |
|
Done! |
|
Thanks! |
Closes #41 (per #41 (comment)).
(Only the second commit matters for the feature, but cleaning the style helped me understand the logic :))