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

added a new test of infeas. prob. with 2 constraints #159

Merged
merged 5 commits into from
Nov 3, 2017

Conversation

IssamT
Copy link
Contributor

@IssamT IssamT commented Nov 2, 2017

replaces #156

cd1 = MOI.get(m, MOI.ConstraintDual(), c1)
cd2 = MOI.get(m, MOI.ConstraintDual(), c2)
bndyd = MOI.get(m, MOI.ConstraintDual(), bndy)
@test (cd2-bndyd)/cd1 ≈ 3 atol=atol rtol=rtol
Copy link
Member

Choose a reason for hiding this comment

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

Could you add tests for the signs of the dual values ?
I would also rather write - 3 cd1 + cd2 ≈ bndyd and 2 cd1 ≈ bndxd to make it clearer why the test should be true by being closer to the dual constraint.

Copy link
Member

Choose a reason for hiding this comment

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

We could also add a test for the objective -7a + 2b > 0

@test cd1 < atol
@test cd2 < atol
@test - 3 * cd1 + cd2 ≈ bndyd atol=atol rtol=rtol
@test 2 * cd1 ≈ bndxd atol=atol rtol=rtol
Copy link
Member

Choose a reason for hiding this comment

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

it should be -bndyd and -bndxd

@test MOI.canget(m, MOI.ConstraintDual(), c1)
cd1 = MOI.get(m, MOI.ConstraintDual(), c1)
cd2 = MOI.get(m, MOI.ConstraintDual(), c2)
bndyd = MOI.get(m, MOI.ConstraintDual(), bndy)
Copy link
Member

Choose a reason for hiding this comment

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

You should define bndxd here

end
end
end

function contlineartest(solver::MOI.AbstractSolver; atol=Base.rtoldefault(Float64), rtol=Base.rtoldefault(Float64))
linear1test(solver, atol=atol, rtol=rtol)
linear2test(solver, atol=atol, rtol=rtol)
Copy link
Member

Choose a reason for hiding this comment

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

You should add linear12test in this function

Copy link
Member

@blegat blegat left a comment

Choose a reason for hiding this comment

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

Test passing with SDPA and CSDP

@test cd2 < atol
@test - 3 * cd1 + cd2 ≈ -bndyd atol=atol rtol=rtol
@test 2 * cd1 ≈ -bndxd atol=atol rtol=rtol
@test -7 * cd1 + 2 * cd2 > -atol
Copy link
Member

@mlubin mlubin Nov 2, 2017

Choose a reason for hiding this comment

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

Shouldn't this be > atol? The dual objective needs to be strictly positive.

Copy link
Contributor Author

@IssamT IssamT Nov 3, 2017

Choose a reason for hiding this comment

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

I m not sure. For me, having a tolerance towards a solver meaning that we accept to have an error but only within that tolerance. Namely, abs(expectedValue - actualValue) <= absoluteTolerance must be true

Copy link
Member

Choose a reason for hiding this comment

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

Here we have no actual value to test, we're checking for strict positivity. If -7 * cd1 + 2 * cd2 == 0 then we do not have a valid infeasibility proof.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is my reasoning:

The test would be correct with 0 tolerance if there exist epsilon > 0 such that -7 * cd1 + 2 * cd2 = epsilon.

Therefore the test would be correct with absoluteTolerance if there exist epsilon > 0 such that -7 * cd1 + 2 * cd2 = actualvalue and abs(epsilon - actualValue) <= absoluteTolerance

Copy link
Member

Choose a reason for hiding this comment

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

We can't let epsilon get arbitrarily small (smaller than absoluteTolerance). The current tests accepts cd1, cd2, bndxd, bndyd = 0.0, 0.0 , 0.0 , 0.0 which is incorrect, and I'd say an important case to catch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand your concern but I don't think using > atol is the correct solution because it is not consistant with the general definition of tolerance ("accepting incorrect answers as long as they are not too far from the correct answer"). It is better to define precisely what atol means for us. Solvers accept many tolerances such as feasibility/optimality tolerance... Since we use only atol everywhere I assume it s the largest one. We can enforce that infeasibility/unbounded ray are correct as well with atol tolerance, when scaled such as the biggest component in the vector has absolute value 1. In this case we need to first scale the ray using infinite norm, and then test the exact values using atol.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @mlubin it should be > atol

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, since you both agree on it.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that this is a nominally improper use of atol. Rescaling the ray and then comparing with a value would also be okay. We are going to have to revisit tolerances in the future anyway: #144.

(this is IMO not the right solution to avoid
the 0 vector being an accepted ray, but
mlubin and blegat agrees on it)
@mlubin mlubin merged commit 7ce48e2 into jump-dev:master Nov 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants