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

WIP: More tests #103

Merged
merged 31 commits into from
Aug 9, 2017
Merged

WIP: More tests #103

merged 31 commits into from
Aug 9, 2017

Conversation

odow
Copy link
Member

@odow odow commented Aug 1, 2017

Port some of the MPB tests from linprog (1 c,d,e from #23) and add tests for the result count attribute #99 (comment)

Closes #105

@odow odow changed the title More contlinear tests More tests Aug 1, 2017
@odow odow changed the title More tests WIP: More tests Aug 1, 2017
@test MOI.cangetattribute(m, MOI.PrimalStatus())
@test MOI.getattribute(m, MOI.PrimalStatus()) == MOI.FeasiblePoint
@test MOI.cangetattribute(m, MOI.ResultCount())
@test MOI.getattribute(m, MOI.ResultCount()) == 1
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure, but maybe we could test for MOI.getattribute(m, MOI.ResultCount()) >= 1 instead of == 1.

@test MOI.getattribute(m, MOI.ObjectiveValue()) ≈ 19 atol=ε

@test MOI.cangetattribute(m, MOI.VariablePrimal(), v)
@test MOI.getattribute(m, MOI.VariablePrimal(), v) ≈ [4,5,1] atol=ε
Copy link
Member

Choose a reason for hiding this comment

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

The problem seems to be degenerate, x = 0, y = 7, z = 1, maybe we could exchange the bounds between x and y

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. I just copied the CPLEX problem

@odow
Copy link
Member Author

odow commented Aug 2, 2017

@joaquimg do you have SOS working in Xpress? I can't get CPLEX to pass the SOSII test with MOI...

It's like I'm adding in the wrong constraint somewhere... Indeed I was...

@odow
Copy link
Member Author

odow commented Aug 2, 2017

This should probably get merged soon as it contains a lot of new tests and various fixes to current ones that are necessary to get passing.

@odow odow mentioned this pull request Aug 2, 2017
13 tasks
@test MOI.getattribute(m, MOI.TerminationStatus()) == MOI.Success
@test MOI.getattribute(m, MOI.DualStatus()) == MOI.InfeasibilityCertificate
@test MOI.cangetattribute(m, MOI.ConstraintDual(), c)
@test MOI.getattribute(m, MOI.ConstraintDual(), c) ≈ -1 atol=ε
Copy link
Member

Choose a reason for hiding this comment

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

What should the dual values be on the bound constraints? These should be tested also.

if MOI.getattribute(m, MOI.ResultCount()) == 1
# solver returned an unbounded ray
@test MOI.getattribute(m, MOI.TerminationStatus()) == MOI.Success
@test MOI.getattribute(m, MOI.PrimalStatus()) == MOI.ReductionCertificate
Copy link
Member

Choose a reason for hiding this comment

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

ReductionCertificate is incorrect here. Looks like we forgot to include a ray status.

Copy link
Member Author

Choose a reason for hiding this comment

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

What exactly is a reduction certificate then? I just assumed it was some fancy conic word for an unbounded ray

Copy link
Member

Choose a reason for hiding this comment

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

Actually the ray status is there and is called InfeasibilityCertificate because a primal unbounded ray is an infeasibility certificate of the dual. Facial reduction certificates are mentioned briefly in the manual; they're for cases of duality failures or unattained solutions which never happen for LP.

@odow odow mentioned this pull request Aug 2, 2017
51 tasks
@joaquimg
Copy link
Member

joaquimg commented Aug 4, 2017

@odow I am planning to add SOS support this week

@@ -26,6 +298,10 @@ function knapsacktest(solver::MOI.AbstractSolver, eps=Base.rtoldefault(Float64))

MOI.setobjective!(m, MOI.MaxSense, MOI.ScalarAffineFunction(v, [5.0, 3.0, 2.0, 7.0, 4.0], 0.0))

if MOI.cansetattribute(m, MOI.VariablePrimalStart(), v)
MOI.setattribute!(m, MOI.VariablePrimalStart(), v, [0.0, 0.0, 0.0, 0.0, 0.0])
Copy link
Member

Choose a reason for hiding this comment

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

Any ideas on how do we check this method is working?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope

@joaquimg
Copy link
Member

joaquimg commented Aug 4, 2017

I remember a bug when modifying quadratic objectives: the quadratic terms were not being re-set to zero. Are we testing that?

@odow
Copy link
Member Author

odow commented Aug 7, 2017

Okay. Before I make this PR any longer, lets get this merged. @blegat or @joaquimg looks like you have write access?

@joaquimg
Copy link
Member

joaquimg commented Aug 8, 2017

Great job! Contlinear and intlinear are all passing in CPLEX? I agree with merging today. @blegat do you have any problem with this?


@test MOI.getattribute(m, MOI.ObjectiveValue()) ≈ 79e4/11 atol=ε
@test MOI.getattribute(m, MOI.VariablePrimal(), x) ≈ 650/11 atol=ε
@test MOI.getattribute(m, MOI.VariablePrimal(), y) ≈ 400/11 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 would use rtol instead of atol here. isapprox is implemented as

abs(x-y) <= atol + rtol*max(abs(x), abs(y)))

where rtol is 1.4901161193847656e-8 for Float64 by default.
For the objective rtol * max(abs(x), abs(y)) is approximately 1e-3 so if atol is like 1e-5 it will be neglibeable.
CSDP fails the test for VariablePrimal/x because epsilon is 1e-7 which means that atol is 1e-7butrtol * max(abs(x), abs(y))is about1e-7too. CSDP has a relative tolerance of1e-7` so it should pass the test but here it is tested for an absolute tolerance of 1e-7.

In fact, it makes more sense to only use atol when one of the two numbers being compared is 0 or very close to 0. Why we use atol everywhere in MOI's tests ? Usually it does not matter much but here since the numbers are large, it does matter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Solvers are able to specify different tolerances... for example https://github.com/JuliaOpt/CPLEX.jl/pull/139/files#diff-fce720c43af3c52c862fd7451c7374b8R30

Copy link
Member

Choose a reason for hiding this comment

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

Yes but the tolerance should be relative, not absolute right ?

Copy link
Member Author

Choose a reason for hiding this comment

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

How about setting atol and rtol?

Copy link
Member

@blegat blegat Aug 9, 2017

Choose a reason for hiding this comment

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

It might be cumbersome since usually the solver just tells its accuracy and we should expect to pass the tests with epsilon = this accuracy.
I would suggest using atol when what we compare to zero and rtol when we compare far from zero. rtol is pretty bad close to zero:

isapprox(1e-9, 1e-10, rtol=1e-2)

So we do

@test x  0 atol=ε
@test x  1 rtol=ε
@test x  y atol=ε rtol=ε # x and y might be close to zero or far from zero, we don't know. I don't know if this case happens in the tests though

What do you think ? Setting atol and rtol might also be a flexible solution.
@joaquimg @joehuchette @chriscoey Any preference ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Have you seen 83f919e?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we can use this solution if everyone agree on it.

@odow
Copy link
Member Author

odow commented Aug 8, 2017

Contlinear and intlinear are all passing in CPLEX?

Contlinear, intlinear, contquadratic, and most of contconic are passing.

@blegat blegat merged commit 24f924b into jump-dev:master Aug 9, 2017
@odow odow deleted the newtests branch October 18, 2017 22:38
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.

None yet

4 participants