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

Add solution_summary function #2512

Merged
merged 17 commits into from
Mar 16, 2021
Merged

Add solution_summary function #2512

merged 17 commits into from
Mar 16, 2021

Conversation

AtsushiSakai
Copy link
Contributor

Hi. First of all, thank you for developing great OSS!! JuMP is one of the greatest OSS I've ever used : ).

I am +1 for #2191, I think solution summary report is very helpful for checking optimization results briefly.
So, I added show_solution_summary function to fix #2191 based on the comment( #2191 (comment)).
I also added tests and docs, and updated manual.

This is the first time for me to contribute JuMP, so any kind of feedback is welcome.

@codecov
Copy link

codecov bot commented Mar 1, 2021

Codecov Report

Merging #2512 (05d42f0) into master (8152c8b) will increase coverage by 1.26%.
The diff coverage is 96.96%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2512      +/-   ##
==========================================
+ Coverage   92.42%   93.69%   +1.26%     
==========================================
  Files          43       43              
  Lines        4688     5313     +625     
==========================================
+ Hits         4333     4978     +645     
+ Misses        355      335      -20     
Impacted Files Coverage Δ
src/print.jl 93.05% <96.96%> (+2.03%) ⬆️
src/_Derivatives/subexpressions.jl 97.61% <0.00%> (-2.39%) ⬇️
src/callbacks.jl 100.00% <0.00%> (ø)
src/indicator.jl 100.00% <0.00%> (ø)
src/complement.jl 100.00% <0.00%> (ø)
src/file_formats.jl 100.00% <0.00%> (ø)
src/_Derivatives/types.jl 100.00% <0.00%> (ø)
src/Containers/no_duplicate_dict.jl 100.00% <0.00%> (ø)
src/_Derivatives/topological_sort.jl 100.00% <0.00%> (ø)
src/lp_sensitivity2.jl 99.32% <0.00%> (+0.02%) ⬆️
... and 31 more

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 8152c8b...05d42f0. Read the comment docs.

Copy link
Member

@odow odow left a comment

Choose a reason for hiding this comment

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

Thanks! A few comments, but this looks nice.

docs/src/manual/solutions.md Outdated Show resolved Hide resolved
docs/src/manual/solutions.md Outdated Show resolved Hide resolved
docs/src/manual/solutions.md Outdated Show resolved Hide resolved
src/print.jl Outdated Show resolved Hide resolved
src/print.jl Outdated Show resolved Hide resolved
src/print.jl Show resolved Hide resolved
src/print.jl Outdated Show resolved Hide resolved
src/print.jl Outdated Show resolved Hide resolved
docs/src/manual/solutions.md Outdated Show resolved Hide resolved
src/print.jl Outdated Show resolved Hide resolved
@AtsushiSakai
Copy link
Contributor Author

Thank you for reviewing. I think I addressed all of your comments and CI was fixed. PTAL.

@AtsushiSakai AtsushiSakai requested a review from odow March 2, 2021 14:12
Copy link
Member

@odow odow left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. I think it will make a great addition.

A couple of very minor things, and one more substantial point about whether we should print values in verbose mode if they have empty names.

docs/src/manual/solutions.md Outdated Show resolved Hide resolved
docs/src/manual/solutions.md Show resolved Hide resolved
src/print.jl Outdated Show resolved Hide resolved
src/print.jl Outdated Show resolved Hide resolved
src/print.jl Outdated Show resolved Hide resolved
@AtsushiSakai AtsushiSakai requested a review from odow March 3, 2021 12:39
Copy link
Member

@odow odow left a comment

Choose a reason for hiding this comment

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

Thanks! I added one thing to only print the variable if the name is non-empty.

Assuming I didn't make a typo, I'll merge this once CI passes.

@odow
Copy link
Member

odow commented Mar 3, 2021

Actually, I have one more thing to bikeshed. I know we have show_constraint_summary, but should we call it print_solution_summary, or even just solution_summary?

@AtsushiSakai
Copy link
Contributor Author

AtsushiSakai commented Mar 4, 2021

I think many Julia codes use print for un-decorated representations and show for decorated representations.
I feel the show_solution_summary, show_constraints_summary , show_objective_function_summary shows decorated representations, so show_*_summary make sense for me. But print_*_summary is also OK.
However, IMO, API name consistency is important. So, either using show_*_summary or deprecating current show_*_summary functions and usingprint_*_summary would be better.

@blegat
Copy link
Member

blegat commented Mar 4, 2021

Maybe solution_summary(model) that would return a struct SolutionSummary; model::JuMP.Model; end ? It prints if it's returned to the REPL but you can also print it with print(solution_summary(model))

@AtsushiSakai
Copy link
Contributor Author

I think @blegat idea is more Julia style. I think I can define SolutionSummary struct and change show_solution_summary to Base.show(summary::SolutionSummary).
However, if we do this, other summary struct should be defined. What do you think? @odow

Copy link
Member

@mlubin mlubin left a comment

Choose a reason for hiding this comment

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

+1 to @blegat's struct proposal. Having a solution_summary doesn't force us to have other summary structs any more than show_solution_summary forces us to have other show_XXX_summary.

test/print.jl Outdated Show resolved Hide resolved
test/print.jl Outdated Show resolved Hide resolved
@AtsushiSakai AtsushiSakai changed the title Add show_solution_summary function Add solution_summary function Mar 7, 2021
@AtsushiSakai
Copy link
Contributor Author

@mlubin Thank you for your comment. I understood. I used struct. PTAL.

@AtsushiSakai AtsushiSakai requested a review from mlubin March 7, 2021 07:02
Copy link
Member

@odow odow left a comment

Choose a reason for hiding this comment

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

@blegat, I don't really understand the motivation for returning a struct just to overload show. solution_summary just prints things.

If we were to return a struct, it should have accessible fields with all the data.

struct SolutionSummary
    solver::String
    # Status
    termination_status::MOI.TerminationStatusCode
    primal_status::MOI.ResultStatusCode
    dual_status::MOI.ResultStatusCode
    raw_status::String
    result_count::Int
    has_values::Bool
    has_duals::Bool
    # Candidate solution
    objective_value::Float64
    objective_bound::Float64
    dual_objective_value::Float64
    primal_solution::Dict{String,Float64}
    dual_solution::Dict{String,Float64}
    # Work counters
    solve_time::Float64
    barrier_iterations::Union{Missing,Int}
    simplex_iterations::Union{Missing,Int}
    node_count::Union{Missing,Int}
end

Then it makes sense to overload show on that.

src/print.jl Outdated Show resolved Hide resolved
docs/src/manual/solutions.md Outdated Show resolved Hide resolved
src/print.jl Outdated Show resolved Hide resolved
src/print.jl Outdated Show resolved Hide resolved
src/print.jl Outdated Show resolved Hide resolved
src/print.jl Outdated Show resolved Hide resolved
@blegat
Copy link
Member

blegat commented Mar 8, 2021

If we were to return a struct, it should have accessible fields with all the data.

Agreed, no need to defer computation to when it's printed so we might as well store them in a struct.

src/print.jl Outdated Show resolved Hide resolved
src/print.jl Outdated
has_duals(model),
objective_value(model),
_try_get(objective_bound, model),
_try_get(dual_objective_value, model),
Copy link
Member

@blegat blegat Mar 10, 2021

Choose a reason for hiding this comment

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

We should have an array here with one entry for i in 1:result_count that contains a struct containing the objective value, solution and dual solution, dual objective value, primal status, dual status, ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have an example code which has result_count is 2 or more? I would like to add a test using it.

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 found this codes. Maybe I can use it.

@testset "ResultCount" begin
m = Model()
@variable(m, x >= 0.0)
@variable(m, y >= 0.0)
@objective(m, Max, x + y)
@constraint(m, c1, x <= 2)
@constraint(m, c2, x + y <= 1)
model = MOIU.Model{Float64}()
MOIU.loadfromstring!(
model,
"""
variables: x, y
maxobjective: x + y
xub: x >= 0.0
ylb: y >= 0.0
c1: x <= 2.0
c2: x + y <= 1.0
""",
)
set_optimizer(
m,
() -> MOIU.MockOptimizer(
MOIU.Model{Float64}(),
eval_objective_value = false,
),
)
JuMP.optimize!(m)
mock = JuMP.backend(m).optimizer.model
MOI.set(mock, MOI.TerminationStatus(), MOI.OPTIMAL)
MOI.set(mock, MOI.ResultCount(), 2)
aff_expr = @expression(m, x + y)
quad_expr = @expression(m, x * y)
nl_expr = @NLexpression(m, log(x + y))
@test JuMP.result_count(m) == 2

Copy link
Member

Choose a reason for hiding this comment

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

Let's just leave this as-is for now. It covers 99.9% of peoples use-cases, and I'm mindful that @AtsushiSakai has put a lot of effort in, and we keep asking for more and more changes.

In future, we could add a alternative_solutions::Vector that is backward compatible. The first result is special as it is the only solution guaranteed to be optimal if TerminationStatus is optimal.

test/print.jl Outdated Show resolved Hide resolved
@odow odow merged commit 8b13032 into jump-dev:master Mar 16, 2021
@odow
Copy link
Member

odow commented Mar 16, 2021

Thanks @AtsushiSakai. It's a very nice contribution.

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.

Feature Request: Report table for query solutions
4 participants