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

Parametrize Problem with a numeric type T #289

Closed
wants to merge 5 commits into from

Conversation

ericphanson
Copy link
Collaborator

Right now, Convex is pretty good about not converting user supplied data to Float64. The only explicit numeric types I can find in Convex are the Float64 sparse arrays made by commands like spzeros. Most of the time, these are multiplied against user input, so e.g. if the user passes in BigFloats, the resulting type will still be a BigFloat.

This isn't the case in conv, which is essentially a bug, since it rules out complex inputs without much reason (#288), but I think in the other atoms it is the case, and I fixed conv here.

At the last step, Convex assembles the objective function and constraints into a standard form to pass to MathProgBase, and it is at this step that Convex builds a sparse matrix A to hold all the numbers associated to the linear part of the constraints, a sparse vector b to hold the affine part, and a sparse vector c to hold the objective function. Right now, all three of these are constructed via spzeros(size...) and hence have type Float64.

Instead, I propose we parametrize Problem with a type T, and construct A, b, and c via spzeros(T, size...), and add a default constructor to allow Problem() to construct a Problem{Float64}() (which has the same behavior as the current Problem struct). I chose T <: Real to avoid confusion: T should be real even if there are complex variables in the problem, since they are transformed to a real form during the problem assembly. I.e. T governs the eltypes of A, b, and c in the standard form.

This has no affect on anyone using the current API, but it allows one to use Problem{BigFloat}(:minimize, ...) to construct problems with more precision that can then be passed to the solvers (none of which currently support high precision, but I'm hoping we can change that this summer). I added some tests to demonstrate that we can pass BigFloats all the way through the problem construction and get them out from conic_problem.

We could also try to deduce the type from the numeric types of the problem data itself, but that seemed like a more complicated change with more possibilities for bugs.

One other relevant thing is that if we do switch to MathOptInterface, we will likely want to use the MathOptInterface.Utilities.@model macro to construct a concrete MathOptInterface model, i.e. a struct MOIModel{T} (analogous to the MathProgBase.conic_model we use now). These models also have a single type parameter governing the model, and all the constraint functions and objective functions have this type. By parametrizing Problem with a type T, when we want to construct an associated MOI model we can use T to construct the appropriate kind of MOI model (i.e., a MOIModel{T}).

To be clear, the change I propose here doesn't really help or hurt our adoption of MOI-- we could adopt MOI independently of the kind of change I propose here, and simply take T=Float64 (since that is the type of our A, b, and c that we would use to construct the MOI objective and constraints). But since MOI already parametrizes their models (at least the ones constructed via @model, which seems like the simplest way to go), it makes sense to me to first parametrize our Problems too.

I also loosened the type restriction on optval. It should be nothing or else whatever the solver passes back, which should be a number but might not be a Float64. I don't think this should affect any current code.

@ericphanson
Copy link
Collaborator Author

We can also remove Float64OrNothing since it is no longer used with the change to the parametrization of optval in Problem. Float64OrNothing is exported though, so that would be an API change.

head::Symbol
objective::AbstractExpr
constraints::Array{Constraint}
status::Symbol
optval::Float64OrNothing
optval::Union{Real,Nothing}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
optval::Union{Real,Nothing}
optval::Union{T,Nothing}

? Otherwise the type parameter isn't actually used.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, I might not be using the type parameters correctly here. I don't think optval should necessarily be of type T, since if the solver returns a Float64, it would be better to not have it converted to e.g. a BigFloat in case T = BigFloat (since then you might not know you only got back a Float64 from the solver). The reason I want to add the type parameter is for line 112 of the proposed version,

function conic_problem(p::Problem{T}) where {T}

That is, the type parameter is to pass the type to conic_problem so that it can use it when constructing A, b, and c in the body of conic_problem:

c = spzeros(T, var_size, 1)
A = spzeros(T, constr_size, var_size)
b = spzeros(T, constr_size, 1)

But since it isn't used to actually parametrize fields of the struct, maybe it should just be another field?

The reason I think it makes sense as a type parameter is that in an ideal world we might have

mutable struct Problem{T<:Real}
    head::Symbol
    objective::AbstractExpr{T}
    constraints::Array{Constraint{T}}
    status::Status
    optval::Union{Real,Nothing}
    model::Union{MOIModel{T}, Nothing}
    solution::Solution
    ... inner constructor
end

where an AbstractExpr{T} is an object that has only children of type AbstractExpr{T} or Constant{T} (or is itself a variable or a Constant{T}). (Actually, to support complex numbers we would want to allow Constant{Complex{T}} or Constant{T}). But right now, we allow Constant's of various types to be mixed together in one problem, so that would be a bigger change. I'm up for making that bigger change though if it's better.

@ericphanson
Copy link
Collaborator Author

Closing in favor of #330 (which was built on top of this)

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.

2 participants