search

darnstrom / daqp

A dual active-set algorithm for convex quadratic programming
MIT License
63 stars 12 forks source link

Support already factored H in DAQPProblem (#54) #59

Open kkofler opened 4 days ago

kkofler commented 4 days ago

include/types.h (DAQPProblem): Add is_factored field and document it.

src/utils.c (update_Rinv): Handle it: adapt the diagonal special case, and skip the factoring in the general case.

Fixes #54.

darnstrom commented 4 days ago

Thank you for this contribution! I think that the is_factored field might serve better in DAQPSettings rather than in the DAQPProblem. The main reason is that I want to keep the main API minimal, and adding the field is_factored would require the user to set it to zero in the "default" case (while having it as zero in the default settings would keep it "invisible" to the average user)

kkofler commented 4 days ago

adding the field is_factored would require the user to set it to zero in the "default" case

The compiler will actually automatically default the member at the end of the struct to 0 if it is not specified (if the initializer is "too short"). (It may or may not warn about that.) That is why I have added it to the end of the struct.

See, e.g., ISO C99 (9899:1999, or WG14/N1124 which is C99+TC1+TC2) 6.7.8§21, or the corresponding paragraph in ISO C90 6.5.7:

If there are fewer initializers in a brace-enclosed list than there are […] members of an aggregate, […] the remainder of the aggregate shall be initialized implicitly the same as objects that have static storage duration.

(The parts I cut out were added in C99 and are not relevant to the struct case.)

So, while we can move it to DAQPSettings, I am not yet convinced that that is an improvement. The flag says how a field in DAQPProblem must be interpreted. Even the dimension of the array H is different based on that flag. Having it in a different struct is just asking for mismatches.

Though in the end it is your code and your decision.