Support TIME_LIMIT in is_solved_and_feasible

[odow]

Originally added in #3668, doesn't look like we discussed TIME_LIMIT.

But this came up last year in a discussion I had with Simon Bowly, where I thought we did support TIME_LIMIT.

And then @ccoffrin just asked me about it as well.

I tend to think that most people who set a time limit and get given a feasible solution would say that is_solved_and_feasible is true.

If we decide to add, there's an open question whether we should fold this into allow_local, or whether we add a new allow_time_limit kwarg.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.58%. Comparing base (6e4427a) to head (4e03c2b).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3915   +/-   ##
=======================================
  Coverage   99.58%   99.58%           
=======================================
  Files          43       43           
  Lines        6059     6059           
=======================================
  Hits         6034     6034           
  Misses         25       25           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[joaquimg]

I do not like this.

Why TIME_LIMIT is more important than ITERATION_LIMIT, NODE_LIMIT or even OBJECTIVE_LIMIT?

I think users needing more details should use the statuses directly.

I we end up adding it, I would certainly I prefer allow_time_limit with a default false.

[ccoffrin]

Don't change the semantics on my account. As long as the documentation is clear, I think this semantics is clean.

Reflecting on my confusion. I think the root of the issue here is solved is a bit of ambiguous term, so I jumped to a conclusion about what it meant without reading the docs. Given that this needs to support both optimal and converged status types I agree solved is a reasonable umbrella term.

In the use-case I encountered, what I needed is an is_feasible or has_solution function, which just checks for primal feasibility. Do we already have a shorthand for that type of test?

[odow]

TIME_LIMIT seems like the one most commonly set by users who expect a solution. ITERATION_LIMIT can happen in interior point-type solvers and tends to indicate failure, not success. NODE_LIMIT and OBJECTIVE_LIMIT are up for debate.

In the use-case I encountered, what I needed is an is_feasible or has_solution function, which just checks for primal feasibility. Do we already have a shorthand for that type of test?

Just define:

has_values(model) = primal_status(model) == FEASIBLE_POINT

We have

JuMP.jl/src/variables.jl

Lines 1916 to 1951 in 99cdc4c

	"""
	has_values(model::GenericModel; result::Int = 1)
	Return `true` if the solver has a primal solution in result index `result`
	available to query, otherwise return `false`.
	See also [`value`](@ref) and [`result_count`](@ref).
	## Example
	```jldoctest
	julia> import HiGHS
	julia> model = Model(HiGHS.Optimizer);
	julia> set_silent(model)
	julia> @variable(model, x);
	julia> @constraint(model, c, x <= 1)
	c : x ≤ 1
	julia> @objective(model, Max, 2 * x + 1);
	julia> has_values(model)
	false
	julia> optimize!(model)
	julia> has_values(model)
	true
	```
	"""
	function has_values(model::GenericModel; result::Int = 1)
	return primal_status(model; result = result) != MOI.NO_SOLUTION
	end

but it's not quite the same thing.

[odow]

Call conclusions were somewhat inconclusive.

@ccoffrin suggested that he really needed only primal_status(model) == FEASIBLE_POINT, which didn't seem to justify a new function.

We also see value in "teaching" users that they need to analyze the statuses in more detail. is_solved_and_feasible is okay for scripts and notebooks, but not really for library code or some sort of meta algorithm.

This PR cannot be merged as-is.

An alternative is:

is_solved_and_feasible(model; allow_time_limit::Bool = false)

but this doesn't help cases like with other limits like SOLUTION_LIMIT.

A second alternative is:

is_solved_and_feasible(model; allowed_termination_status = [OPTIMAL, LOCALLY_INFEASIBLE])
is_solved_and_feasible(model; allowed_termination_status = [OPTIMAL, LOCALLY_INFEASIBLE, TIME_LIMIT])

but this requires the user to know the various statuses, and it's a bit cumbersome. And something like this is painful:

is_solved_and_feasible(model; additionally_allowed_termination_status = [TIME_LIMIT])

One option is:

if is_solved_and_feasible(model) ||
   (termination_status(model) == TIME_LIMIT && primal_status(model) == FEASIBLE_POINT)
    # ...
end

@blegat also sugggested is_solved(model) and is_feasible(model) to split the logic, which would enable:

if (is_solved(model) || termination_status(model) == TIME_LIMIT) && is_feasible(model)
    # ...
end

But this is just adding more complexity when at this point the user really needs to write proper logic for handling the statuses. Yes, this means duplicated code, but that's not the end of the world because it makes things very clear about what is allowed etc.

We have https://jump.dev/JuMP.jl/stable/manual/solutions/#Recommended-workflow, which I think stands up pretty well.

[odow]

Perhaps this PR can just be closed. #3925 tweaks the docstring of is_solved_and_feasible.

[joaquimg]

I would prefer a list of accepted statuses instead of more keywords

[odow]

Suggested name for the keyword?

[joaquimg]

allow_status = (), and I would suggest a tuple since it is non-allocating. But accepting any collection that work with in should be fine

[joaquimg]

I do not see a reason to force a Vector, we should at least allow tuples.

[odow]

I really want to minimise the number of input types we support. This is part of the MethodError principle. Is there a good argument not to restrict to Vector? The allocations here are minimal, and checking this status should be so minor in comparison to solving the problem.

[joaquimg]

Suggested change

	additional_termination_statuses::Vector{TerminationStatusCode} = TerminationStatusCode[],
	additional_termination_statuses::Union{
	Vector{TerminationStatusCode},
	NTuple{N,TerminationStatusCode}} = (),

[joaquimg]

The Method error should be very clear with this suggestion. No weird tuples would be allowed.

Allocations are minimal, but there is no reason for them. Minimal allocations in a loop become big allocations.

[odow]

Who's calling this in a loop? And if they are, they cache the vector once outside the loop.

[odow]

I'm also somewhat in favor of just closing this PR and leaving the behavior as-is for now.

[odow]

Closing for now. I'll make a mental note to re-open this if the question comes up again in the future.

And if you, reader from the future, are interested in this feature, please comment below and I will re-open.