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

Potential typo when computing primal_feasibility_in_rhs_0 in global_primal_residual? #254

Closed
oswinso opened this issue Aug 13, 2023 · 2 comments

Comments

@oswinso
Copy link

oswinso commented Aug 13, 2023

I think maybe there is a typo when computing primal_feasibility_in_rhs_0 in the global_primal_residual function when there are box constraints?

primal_feasibility_in_rhs_0 = std::max(
primal_feasibility_in_rhs_0, infty_norm(qpresults.si.tail(qpmodel.dim)));

Mathematically, my understanding is that this is used for computing the relative errors, and should be

$$ \text{primal\_feasibility\_in\_rhs\_0} = \max\{ \lVert C x \rVert_{\infty}, \lVert x \rVert_{\infty} \} $$

However, the problem right now is that qpresults.si.tail(qpmodel.dim) does not contain $x$. It seems like this is not assigned in this function, and either contains zero or some other expression, i.e., from here

// contains now : C(x+alpha dx)-l + z_prev * mu_in
qpresults.si += alpha * Cdx;

This seems like a typo?

Perhaps it should instead be

primal_feasibility_in_rhs_0 = std::max(
      primal_feasibility_in_rhs_0, infty_norm(qpwork.primal_residual_in_scaled_up.tail(qpmodel.dim)));

which does contain $\lVert x \rVert_{\infty}$, as assigned from earlier in the function?

qpwork.primal_residual_in_scaled_up.tail(qpmodel.dim) = qpresults.x;
// qpwork.primal_residual_in_scaled_up.tail(qpmodel.dim).array() *=
// qpwork.i_scaled.array();
ruiz.unscale_primal_in_place(VectorViewMut<T>{
from_eigen, qpwork.primal_residual_in_scaled_up.tail(qpmodel.dim) });

@Bambade Bambade mentioned this issue Sep 10, 2023
@Bambade
Copy link
Collaborator

Bambade commented Sep 10, 2023

Dear @oswinso,
Thanks a lot for reporting this typo. qpresults.si.tail(qpmodel.dim) was indeed not assigned at this stage.
There are several ways for computing relative errors. One usual way (adapted to the box constraint case) is to indeed use $||x||_\infty$ and also the norm of its projection onto the box constraints. I have added these two in PR #258.

jcarpent added a commit that referenced this issue Sep 10, 2023
@jcarpent
Copy link
Member

Solved via #258.

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

No branches or pull requests

3 participants