Skip to content

Fix column bounds in findLowerLevelSolImprovingDirectionIC()#143

Open
febattista wants to merge 1 commit into
coin-or:stable/1.2from
febattista:fixColBounds
Open

Fix column bounds in findLowerLevelSolImprovingDirectionIC()#143
febattista wants to merge 1 commit into
coin-or:stable/1.2from
febattista:fixColBounds

Conversation

@febattista
Copy link
Copy Markdown

@febattista febattista commented Oct 3, 2024

Guarantee in the MILP subproblem for finding an ImprovingDirection, 0 is always a feasible value for each variable.

EDIT: I'm reporting a more detailed description of the issue.

In the function intersectionCuts(), the values of lpSol get rounded as follows:

for(i = 0; i < numStruct; i++){
    value = lpSol[i];
    if(fabs(floor(value + 0.5) - value) <= etol){
		lpSol[i] = floor(value + 0.5);
    }
}

where etol = 1e-5. Now, this is supposed to round those values while maintaining the solution feasible. However, the value that create the issue is -0.000012204442799038335, which of course "should be" 0 (since x >= 0), but bypasses the rounding since the absolute value is above etol.
Then, the same situation comes up when findLowerLevelSolImprovingDirectionIC() sets the bounds, since that value has not changed.

Comment thread src/MibSCutGenerator.cpp
// feb223: w = 0 should be always a feasible direction
nSolver->setColLower(i, CoinMin(ceil(origColLb[colIndex] - value -
localModel_->etol_), 0.0));
nSolver->setColUpper(i, CoinMax(floor(origColUb[colIndex] - value +
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, I really don't understand code editors the introduce weird indentation. We need a workflow that runs the code through a formatter on every commit.

Copy link
Copy Markdown
Member

@tkralphs tkralphs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem right, now that I'm looking at it. It's a numerical issue, not an issue of forcing 0 to be feasible. We shouldn't have to force this by doing min/max. The issues is more like that the floor operator just isn't correct if the thing you're applying the 'floor` to might already be below what it's true value is supposed to be. There are a couple of possibilities.

  • If value is supposed to be integer, then rounding it before setting bounds should be valid and would fix this particular issue. Of course, technically, you should round the whole solution and make sure it remains feasible, yada, yada.
  • Alternatively, if the bound is supposed to be integer, it seems like rounding it should also be correct.

With something like a cut, you want to be conservative in rounding, always rounding the direction that relaxes the cut, to avoid removing a feasible solution that should not be removed. But here, we also want to be careful not to generate directions that actually aren't feasible, so you don't want a bound that is too loose either. I guess you really need the bound to be the exact correct value.

The point is that I guess this is not just an issue of whether zero, in particular, is feasible or infeasible, but is an issue of having the correct bounds in any case, not just when they are zero.

@tkralphs
Copy link
Copy Markdown
Member

@febattista I'm trying to clean up some issues and PRs, can we revisit this when you have a moment to remind yourself?

@febattista
Copy link
Copy Markdown
Author

febattista commented Dec 17, 2025

Okay, I'm trying to do some house chores regarding this. Unfortunately, I couldn't trace back which instance was causing this issue (...sigh). However, from a personal conversation and from what I remember this is "just" a numerical issue related to the LP solver returning a solution that does not satisfy the constraints within the tolerance in this specific case.

I agree that forcing 0 to be a feasible bound for a direction may not be the way to deal with this and I would be okay to revert this change to what it was (not forcing 0). This situation seemed only to arise in one isolated case. I personally think that the way the lower and upper bounds were originally computed is the right way to do it (assuming an LP solution satisfying the tolerance).

@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Federico Battista seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

3 participants