osqp / qdldl

A free LDL factorisation routine
Apache License 2.0
79 stars 39 forks source link

custom number types #3

Closed mlubin closed 6 years ago

mlubin commented 6 years ago

If I understand the readme correctly (https://github.com/oxfordcontrol/qdldl#including-the-header), there's a bit of a design issue with the custom number types.

At the point when a user redefines these types when including the header, the library has already been compiled using some fixed set of integer types. Using different definitions at include time will just result in linking errors or crashes when compiler finds functions with unexpected argument types. The choice of number types needs to be a compile-time option, e.g., controlled by a config file or cmake config.

Another option is to automatically compile all reasonable combinations of integer/float types and expose them as, e.g., QDLDL_solve_float64_int64. This requires a bit more work but it would allow users to compile a binary that provides multiple precision options at runtime.

goulart-paul commented 6 years ago

I'm not necessarily against doing this, but note that I haved use bool, float and int types in the factorisation to keep the memory footprint small. If we are to enumerate overall a variety of input types then we should probably replace bool with int to minimize the number of combinations.

mlubin commented 6 years ago

I'd agree that there's no real need to let the bool type vary. int (or something else set at compile-time config) is a reasonable choice.

goulart-paul commented 6 years ago

I would be happy to insist that the boolean type should always be of type bool (or _Bool), but that doesn't exist in C89. The reason for making it user definable was largely to preserve the option to define it as 'int' if when there is no native boolean type.

If it is to be the same type as the other ints (most likely int64), then there is a fairly large memory penalty if you have a really huge matrix with very low density.

bstellato commented 6 years ago

Another possibility is to specify the types as CMakeLists.txt options in a qdldl_configure.h.in file as done for osqp here. The same could be done with the bool/int decision for memory usage. What do you think?

mlubin commented 6 years ago

Given that C89/C99 support is known at compile time, it's quite reasonable to let this be a config option or just switch on int/bool with macros based on the value of __STDC_VERSION__.

bstellato commented 6 years ago

This should be fixed in 2644bc2. CMake flags for doubles and integers + __STDC_VERSION__ check for bools.