-
Notifications
You must be signed in to change notification settings - Fork 87
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
AbstractVector for list of terms in functions #525
Comments
Using non-concrete vector types may cause a performance hit, not to mention significant code design and readability implications. For performance, we need to have a comprehensive benchmark suite before making a change like this.
I don't understand this use case. Why would you need a
The iterators could also be redesigned to return a hypothetical
I didn't understand the linked comment. I, for one, would be pretty unhappy as a solver wrapper author if users are allowed to provide input using arbitrary data structures. It pushes a lot of complexity into the solver interfaces that really shouldn't be needed. I wouldn't want users complaining to me that there's a bug in the solver wrapper because it doesn't handle
This argument needs benchmarks to be convincing. I should also note that I'm not going to consider nonessential breaking changes in MOI until after JuMP 0.19 is close to being ready to release. Every MOI version bump at this stage seems to translate into a ~1 month delay on the JuMP 0.19 release. |
The solver interface only uses the AbstractVector interface so the solver interface code shouldn't need any change
I totally agree with this, we should add a breaking label for breaking changes |
We discussed this on April's developer call. This can be implemented in a non-breaking future release by introducing a new function type, and having something like Removing from 0.10 milestone and |
Closing in favor of #863 We should consider things holistically if we're going to redesign the functions. |
The functions currently enforces the list of terms to be a
Vector
, e.g.However, the fact that it is a
Vector
and not anyAbstractVector
does not help the solver wrappers in any way. Before we add the term types and we had different field for the coefficients and variables, we could have solver wrappers that pass directly the coefficient vector by pointer to the C-interface but won't do that anymore. It will need to iterate through the vector to do some transformation anyway so I suspect that if we change this to AbstractVector, it won't require any change from the solver wrappers.The tricky point to discuss is how to update the MOIU model. We have a few choices:
Vector
terms and collect the terms if a function is passed with AbstractVectorScalarAffineFunction{T, VT <: AbstractVector{T}}
, we could defineconst StandardScalarAffineFunction{T} = ScalarAffineFunction{T, Vector{T}}
and use@model ... (StandardScalarAffineFunction, ...) ...
to do 1) and do@model ... (ScalarAffineFunction, ...) ...
to do 2).What do you think ?
The advantages of using AbstractVector instead of Vector are numerous:
The text was updated successfully, but these errors were encountered: