Skip to content

Conversation

blegat
Copy link
Member

@blegat blegat commented Aug 16, 2018

Closes #681

@codecov-io
Copy link

codecov-io commented Aug 16, 2018

Codecov Report

Merging #473 into master will increase coverage by 0.06%.
The diff coverage is 97.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #473      +/-   ##
==========================================
+ Coverage   93.83%   93.89%   +0.06%     
==========================================
  Files          54       54              
  Lines        5677     5754      +77     
==========================================
+ Hits         5327     5403      +76     
- Misses        350      351       +1
Impacted Files Coverage Δ
src/attributes.jl 93.05% <100%> (+0.09%) ⬆️
src/Test/contlinear.jl 100% <100%> (ø) ⬆️
src/Utilities/results.jl 90.43% <100%> (+3.07%) ⬆️
src/Test/contconic.jl 100% <100%> (ø) ⬆️
src/Utilities/mockoptimizer.jl 90.52% <83.33%> (-0.34%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c1d8f6a...1e9be17. Read the comment docs.

@mlubin
Copy link
Member

mlubin commented Aug 16, 2018

How much breakage will this cause in the solver interfaces? Are we sure that the solvers can implement this? Do we really want the fallback for ObjectiveBound to be such an expensive operation? The dual objective isn't even a valid bound on the objective if there are violations of dual feasibility. (Consider renaming this use of this attribute to DualObjective.) I'd tempted to say that we should be honest that the solver doesn't have one available.

@joaquimg @odow

@blegat
Copy link
Member Author

blegat commented Aug 16, 2018

If we go in this direction, then we should do

  • PrimalObjective(result), objective value for VariablePrimal(result)
  • DualObjective(result), objective value for ConstraintDual(result)
  • ObjectiveUpperBound(), best upper bound found for objective
  • ObjectiveLowerBound(), best lower bound found for objective

The reason, the first and last one are merged into ObjectiveValue is that we assume that the VariablePrimal returned is the one which has the best objective value upper bound but since we allow multiple VariablePrimal this is kind of ambiguous.

@mlubin
Copy link
Member

mlubin commented Aug 16, 2018

If we go in this direction, then we should do

Maybe we shouldn't go in this direction then. I'm not excited about bikeshedding really basic definitions of objective values at the moment. The current ones are workable. Realistically, doing so will probably delay JuMP 0.19 by two weeks by the time we agree on new definitions and propagate them everywhere.

@blegat blegat mentioned this pull request Mar 2, 2019
@blegat blegat changed the title Add tests and fallback for ObjectiveBound Add DualObjectiveValue Apr 18, 2019
@blegat blegat added this to the v0.9 milestone Apr 19, 2019
@blegat
Copy link
Member Author

blegat commented Apr 20, 2019

Ready for review :)

@blegat
Copy link
Member Author

blegat commented Apr 27, 2019

Any objection to this new attribute ?

@blegat blegat merged commit 837d879 into master May 5, 2019
@odow odow deleted the bl/objbound branch June 15, 2019 15:45
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.

Rework objective results
3 participants