Skip to content

Comments

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

Open
kkofler wants to merge 1 commit intodarnstrom:masterfrom
kkofler:factored-hessian
Open

Support already factored H in DAQPProblem (#54)#59
kkofler wants to merge 1 commit intodarnstrom:masterfrom
kkofler:factored-hessian

Conversation

@kkofler
Copy link

@kkofler kkofler commented Nov 22, 2024

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.

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 darnstrom#54.
@darnstrom
Copy link
Owner

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
Copy link
Author

kkofler commented Nov 23, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Constrained least-squares/LDP (factored QP)

2 participants