L-infinity Norm Termination Criteria#43
Conversation
ZedongPeng
left a comment
ZedongPeng
left a comment
There was a problem hiding this comment.
Thank you for the PR! Supporting L_INF is indeed a great addition for robustness and consistency with other solvers.
However, regarding the API design, let's use a specific option instead of a T/F toggle. I recommend naming it optimality_norm with values L2 and L_INF. This aligns better with our design standards and makes it easier to extend later.
|
The API has been updated, please let me know if this is what you had intended or if you had something else in mind. |
ZedongPeng
left a comment
ZedongPeng
left a comment
There was a problem hiding this comment.
Thank you @lb3825. I left some feedback on the interface/API design. I know these design choices can be subjective, so feel free to discuss or push back if you have other ideas.
src/cli.c
Outdated
| {"verbose", no_argument, 0, 'v'}, | ||
| {"time_limit", required_argument, 0, 1001}, | ||
| {"iter_limit", required_argument, 0, 1002}, | ||
| {"linf_norm", no_argument, 0, 'f'}, |
There was a problem hiding this comment.
This line should be removed.
src/solver.cu
Outdated
| if (val > max_val) max_val = val; | ||
| } | ||
| state->objective_vector_norm = sqrt(sum_of_squares); | ||
| state->objective_vector_norm_inf = max_val; |
There was a problem hiding this comment.
I don't think we need to keep both objective_vector_norm and objective_vector_norm_inf. Let's only keep objective_vector_norm and the value of it depends on optimality_norm.
src/utils.cu
Outdated
| state->absolute_primal_residual / (1.0 + state->constraint_bound_norm); | ||
| state->relative_dual_residual = | ||
| state->absolute_dual_residual / (1.0 + state->objective_vector_norm); | ||
| if (state->optimality_norm == NORM_TYPE_L_INF) { |
There was a problem hiding this comment.
Once constraint_bound_norm_inf is removed, the if-else condition can be simplified here.
src/cli.c
Outdated
| {"sv_max_iter", required_argument, 0, 1011}, | ||
| {"sv_tol", required_argument, 0, 1012}, | ||
| {"eval_freq", required_argument, 0, 1013}, | ||
| {"optimality_norm", required_argument, 0, 1014}, |
There was a problem hiding this comment.
For the CLI, we can use opt_norm here for consistency.
README.md
Outdated
| | `-v`, `--verbose` | `flag` | Enable verbose logging. | `false` | | ||
| | `--time_limit` | `double` | Time limit in seconds. | `3600.0` | | ||
| | `--iter_limit` | `int` | Iteration limit. | `2147483647` | | ||
| | `--optimality_norm` | `int` | Norm for optimality criteria: L2 (0) or L_INF (1) | `0` (L2) | |
There was a problem hiding this comment.
It might be better to use
L2andL_INF(orLINF)- or
l2andl_inf(orlinf) (I personally prefer this one)
instead of using 0 and 1 here.
python/cupdlpx/PDLP.py
Outdated
| UNSPECIFIED = -1 | ||
|
|
||
| # Norm types for optimality criteria | ||
| L2 = 0 |
There was a problem hiding this comment.
I suggest we remove these constants to keep the namespace clean. Users usually prefer passing strings like l2 or linf, which we can map to the C enums inside the wrapper.
|
Hello. The proposed API is definitely much more convenient (should of given it a bit more thought initially, apologies for that). I think all of the requested changes have been made, let me know if there is anything else you would like me to change. Additionally, I had ended up using |
|
Hi @lb3825. Thank you so much for your contribution. Merry Christmas 🎄 |
|
Hi @lb3825. The PR looks great. I made a couple of small changes:
I’m running benchmarks now; once they’re done, this PR should be ready to merge. Thanks again for the contribution! |
2 participants
Added a T/F setting to allow the user to choose whether to have the termination criteria be based off of the infinity norm or the L-2 norm. This is to be more robust and consistent with other solvers who commonly utilize the infinity norm in their termination criteria.