fix(tuneParam): correct csense sizing, safer arg handling, no global solver swap#2652
Conversation
…solver swap
- size(LPProblem.A) returned a 2-vector, then csense(1:nMet,1)='E' used
it as a length, producing an incorrectly sized csense. Use
size(LPProblem.A, 1).
- Apply cpxControl.tune.{timelimit,repeat,display} only after
contFunctName has been resolved into a struct. The previous code
assigned .tune fields onto contFunctName before checking its type, so
passing a function-name string (the documented alternative) crashed.
- Permanent global side-effect: changeCobraSolver('ibm_cplex') silently
switched the user's configured LP solver during availability probing.
Restore the previous CBT_LP_SOLVER after the check.
- Pre-allocate b_L/b_U as -inf/+inf vectors of the correct length so
rows whose csense character is missing get well-defined bounds rather
than undefined trailing zeros.
- Wrap the CPLEX init failure as a typed MException with the original
exception attached via addCause (was: bare error, original cause lost).
- Replace eval-based dotted-path read/write of CPLEX params with
helpers using setfield/getfield (no eval, no silent injection risk).
- Treat empty contFunctName as struct() rather than [] so the function
always returns a struct as documented.
- Convert silent 'Optimisation failed.' fprintf into a proper warning
with an identifier (tuneParam:tuneFailed).
Summary:
Failed Tests:
Github Test Reporter by CTRF 💚 |
|
Hi, |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f47a8e0735
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| b_L = -inf(nRows, 1); | ||
| b_U = inf(nRows, 1); | ||
| if ~isempty(csense) |
There was a problem hiding this comment.
b_L/b_U are initialized to -inf/+inf and only constrained inside if ~isempty(csense). If LPProblem.csense is present but empty (for example '' with a non-empty A), this branch is skipped and every row is effectively free (-inf <= A*x <= inf), so tuning runs on the wrong problem while silently ignoring A/b. The previous implementation failed loudly in this case; this change introduces a silent correctness failure. Please add a guard for empty csense with nonzero rows (or default those rows to equality bounds).
Useful? React with 👍 / 👎.
|
Hi @farid-zare , yes I am on it! |
Address Codex review (PR opencobra#2652): when LPProblem.csense was present but empty (e.g. ''), the bounds branch was skipped, leaving every row at [-inf, +inf] and silently tuning a free LP. Treat empty csense the same as a missing csense (default to equality, matching existing behaviour), and error out when csense length disagrees with size(A,1). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary:
Failed Tests:No failed tests ✨ Github Test Reporter by CTRF 💚 |
2 participants
Summary
Audit of
tuneParam(CPLEX parameter tuner) surfaced several correctness and robustness issues. This PR fixes them with minimal changes and preserves the documented public interface.Fixes
size(LPProblem.A)used as a scalar length. Whencsenseis missing, the code didnMet = size(LPProblem.A); LPProblem.csense(1:nMet,1) = 'E';. Becausesizereturns a 2-vector,1:nMetcollapses to1:size(A,1)only by accident and the assignment behaves unpredictably. Replaced withsize(LPProblem.A, 1)..tune.*fields written beforecontFunctNameis parsed. The previous code didcontFunctName.tune.timelimit = timelimitbefore checking whethercontFunctNamewas a struct. Passing a function-name string (the documented alternative usage, e.g.'CPLEXParamSet') therefore crashed. Now we resolvecontFunctNameto a struct first, then apply the tune overrides.if ~changeCobraSolver('ibm_cplex')was used as an availability check, but it permanently changesCBT_LP_SOLVER. The function now snapshots and restores the previous LP solver after the probe, and useschangeCobraSolver(..., 'LP', 0)to suppress noisy prints.b_L/b_U. The original code assigned only into rows whosecsensematched 'E'/'G'/'L'; rows with any other character were left as undefined trailing zeros. Now both vectors start as-inf/+infof lengthsize(A,1).MException('tuneParam:cplexInit', ...)andaddCause(ME)so the underlying CPLEX error is preserved.eval-based dotted-path access. Reading/writing CPLEX parameters viaeval('optimParam.' paramPath{i} '=ILOGcplex.Param.' paramPath{i} '.Cur')is fragile and unsafe. Replaced withsetfield/getfieldhelpers that walk the dotted path explicitly.contFunctNamepreviously madeoptimParam = []; now it is initialised tostruct()so the function always returns a struct as documented.fprintf('Optimisation failed.')turned into a realwarning('tuneParam:tuneFailed', ...)with an identifier callers can suppress or trap.Backward compatibility
tuneParam(LPProblem, contFunctName, timelimit, nrepeat, printLevel)is unchanged.contFunctNamestill accepts a struct, a function-name string, or empty.tuneParamForModelis unaffected.Files
src/base/solvers/cplex/tuneParam.m— all fixes above. No public API changes.