Skip to content

remove assumption of integer objective, fixes #23 - #24

Open
rowtricker wants to merge 3 commits into
coin-or:developmentfrom
rowtricker:allowNonIntegerObjective
Open

remove assumption of integer objective, fixes #23#24
rowtricker wants to merge 3 commits into
coin-or:developmentfrom
rowtricker:allowNonIntegerObjective

Conversation

@rowtricker

@rowtricker rowtricker commented Nov 25, 2016

Copy link
Copy Markdown

fixes #23

This does make that the incumbent solution is now stored as a double. Alternatively, to prevent a loss of precession, it might be better to store an integer if the property is selected. Any thoughts on this?

&& Math.floor(node.bound + config.PRECISION) <= lowerBoundOnObjective);
} else {
return optimizationSenseMaster == OptimizationSense.MINIMIZE
&& node.bound >= upperBoundOnObjective

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this bit is tricky. It doesn't take the precision into account. I need to read up on this topic to decide what is the best comparison.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I was also not entirely sure what to do here either. However, allowing to deviate by the precision would mean that this would be allowed recursively if the nodes are pruned. Hence, the final precision could turn out significantly worse than the given precision. That was my reason for not taking it into account here.

public final double nodeBound;
/** Best integer solution discovered so far **/
public final int bestIntegerSolution;
public final double bestIntegerSolution;

@jkinable jkinable Nov 28, 2016

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this variable should probably be renamed, as it is no longer an integer solution. I would change it to:
objectiveIncumbentSolution (that would probably make it more consistent as well)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I agree, the term integer solution is somewhat vague anyhow as the integrality could refer to both the objective value as well as the integrality of the solution itself. I will change this.

@@ -46,7 +46,7 @@ public class PruneNodeEvent<T extends ModelInterface, U extends AbstractColumn<T
* @param nodeBound Bound on the node
* @param bestIntegerSolution Best integer solution discovered thus far

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

description needs update

@@ -33,7 +33,7 @@ public class StartEvent
* Best available integer solution at the start of the Branch-and-Price or Column generation

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

descr needs update

@@ -77,7 +77,7 @@ public class ColGen<T extends ModelInterface, U extends AbstractColumn<T, V>,
* is a maximization problem, the Colgen procedure is terminated if
* {@code floor(boundOnMasterObjective) <= cutoffValue}.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

desr needs update

protected double cutoffValue;
/**
* Bound on the best attainable objective value from the master problem. Assuming that the
* master is a minimization problem, the Colgen procedure is terminated if

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

descr needs update

return Math.floor(boundOnMasterObjective + config.PRECISION) <= cutoffValue;
} else {
if (optimizationSenseMaster == OptimizationSense.MINIMIZE)
return boundOnMasterObjective >= cutoffValue;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Check: how to handle precision correctly.

protected String instanceName;
/** Best integer solution obtained thus far **/
protected int bestIntegerSolution;
protected double bestIntegerSolution;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Best to rename this to: objectiveIncumbentSolution
Also, please update descr

public final boolean EXPORT_MODEL;
/** Define export directory for master models. Default: ./output/masterLP/ **/
public final String EXPORT_MASTER_DIR;
/** Defines if an integer solution has an integer objective. Default = true */

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I suggest to be a bit more verbose here:
Defines whether the objective value of any feasible solution is an integer value. More precisely, this value should be set to true if all coefficients in the objective function are integer values, and all variables in the objective function are integer variables. This parameter influences the rounding and pruning behavior in a Branch-and-Price application.

double solution = this.solveTSPInstance(tsp);
System.out.println("Solution for : " + instance + " is: " + solution);
Assert.assertEquals(solution, instances.get(instance).intValue());
Assert.assertEquals(solution, instances.get(instance).intValue(), 0.000001);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Use Config.PRECISION

@rowtricker

Copy link
Copy Markdown
Author

All comments (that can be directly fixed on my side) have been added in the last commit.

@jkinable

Copy link
Copy Markdown
Collaborator

I still haven't found a good solution on how to handle the precision correctly. This will require some more digging...

@rowtricker

Copy link
Copy Markdown
Author

I understand. Would it be a good idea to take a quick glance at how related frameworks are handling this?

@rowtricker

Copy link
Copy Markdown
Author

I found an additional assumption of an integer objective in AbstractBranchAndPrice, which would throw an exception in case the objective is non-integer when trying to round. It has been addressed by the above commit.

@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.
You have signed the CLA already but the status is still pending? Let us recheck it.

@ncabrera10

Copy link
Copy Markdown

Hi,

I'm facing a similar problem. What checks are missing to merge this with the master branch?

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.

4 participants