Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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