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

reset Clp termination code to "time limit reached" when seeing "iteration limit reached" in dual simplex resolve #199

Merged
merged 4 commits into from
Mar 20, 2024

Conversation

yuxies
Copy link
Contributor

@yuxies yuxies commented Nov 30, 2023

Change the return code in dual_simplex from LP_D_ITLIM to LP_TIME_LIMIT if use Clp.

  • Clp may return "maximum iteration limit reached" flag when it is actually timeout (see ClpModel::hitMaximumIterations()).

@yuxies yuxies marked this pull request as draft December 1, 2023 03:52
@yuxies yuxies marked this pull request as ready for review December 1, 2023 03:59
@tkralphs
Copy link
Member

tkralphs commented Mar 4, 2024

@yuxies I don't think we can just always LP_TIMELIMIT here because there are checks in other places for LP_D_ITLIM (like in strong branching). Can we try to distinguish in someway, such as checking whether we have set an iteration limit and or are expecting that return code for some reason? Maybe it's better to do this on the MibS side, since we know we are only expecting a timeout there and not an iteration limit.

@yuxies
Copy link
Contributor Author

yuxies commented Mar 4, 2024

Can we try to distinguish in someway, such as checking whether we have set an iteration limit and or are expecting that return code for some reason?

The change here is inside the function int dual_simplex(LPdata *lp_data, int *iterd), after si->resolve(); so the changed line will only affect the immediate result of the dual simplex solving itself, when Clp is used. And there would not be any misunderstanding, since any other term code from Clp has its own correct mapping, and we are not expecting LP_D_ITLIM as a Clp dual simplex term result.

@tkralphs
Copy link
Member

tkralphs commented Mar 4, 2024

Can we try to distinguish in someway, such as checking whether we have set an iteration limit and or are expecting that return code for some reason?

The change here is inside the function int dual_simplex(LPdata *lp_data, int *iterd), after si->resolve(); so the changed line will only affect the immediate result of the dual simplex solving itself, when Clp is used. And there would not be any misunderstanding, since any other term code from Clp has its own correct mapping, and we are not expecting LP_D_ITLIM as a Clp dual simplex term result.

If I understand correctly, this is not true. We are indeed expecting LP_D_ITLIM as a Clp dual simplex result in some places, as I mentioned. Just search for LP_D_ITLIM in the source code.

@yuxies
Copy link
Contributor Author

yuxies commented Mar 4, 2024

Ok, maybe we can add another condition before this line "checking whether we have set an iteration limit". Then the change will look like:

int itlim_check = -1;
retval=si->getIntParam(OsiMaxNumIteration, itlim_check);
if(itlim_check >= LP_MAX_ITER){ term = LP_TIME_LIMIT; }

@tkralphs
Copy link
Member

tkralphs commented Mar 6, 2024

Hmm, this is probably reasonable. The only edge case would b if an iteration limit is set, but the time limit is nevertheless being reached. Can we check the actual number of iterations or the actual time to see which is being triggered?

@yuxies
Copy link
Contributor Author

yuxies commented Mar 6, 2024

Just change the condition to check the actual iteration number.

@tkralphs
Copy link
Member

tkralphs commented Mar 7, 2024

Yes! But could you open this against master? Then I will cherry pick it over to 5.7.

@yuxies yuxies changed the base branch from stable/5.7 to master March 7, 2024 20:27
@yuxies yuxies changed the base branch from master to stable/5.7 March 7, 2024 20:28
@yuxies yuxies marked this pull request as draft March 7, 2024 20:29
@yuxies yuxies changed the base branch from stable/5.7 to master March 7, 2024 20:43
@yuxies
Copy link
Contributor Author

yuxies commented Mar 7, 2024

Rebased the changes onto the master branch.

Note: there is a similar case switch in this function int initial_lp_solve (LPdata *lp_data, int *iterd){} that may be affected by the Clp termination code issue.

@yuxies yuxies marked this pull request as ready for review March 7, 2024 20:49
@tkralphs
Copy link
Member

Yes, I guess we need to make the same change in initial_lp_solve() and also in solve_hotstart. Can you do that and we can finally put this to bed?

@tkralphs
Copy link
Member

OK, looks good!

@tkralphs tkralphs merged commit 3bd74ee into coin-or:master Mar 20, 2024
12 of 14 checks passed
tkralphs pushed a commit that referenced this pull request Mar 24, 2024
…turns "iteration limit reached" in dual simplex resolve (#199)

(cherry picked from commit 3bd74ee)
Signed-off-by: Ted Ralphs <[email protected]>
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.

2 participants