Updates to MOI v0.9 #144
Updates to MOI v0.9 #144
Conversation
src/types.jl
Outdated
| status::Symbol | ||
| ret_val::Int | ||
|
|
||
| function Solution(x::Array{Float64, 1}, y::Array{Float64, 1}, s::Array{Float64, 1}, ret_val::Int) | ||
| function Solution(x::Array{Float64, 1}, y::Array{Float64, 1}, s::Array{Float64, 1}, info::SCSInfo, ret_val::Int) |
There was a problem hiding this comment.
please update status_map (above) according to https://github.com/cvxgrp/scs/blob/d3d1d4c49b0aede0ec491fe06c99aef5cfe598c7/include/glbopts.h#L18 (or similar in MOI_wrapper.jl)
I once opened a pull with this, but this was in pre-MOI times, so we left it for later, maybe now is the time;
maybe it's a good idea to share the definition with MOI?
There was a problem hiding this comment.
The status should be one of the MPB statuses. For custom status described by SCS, this should be raw_status(info).
We should remove this from the struct since it is tied to MPB
There was a problem hiding this comment.
Ah no I see, we don't handle outside [-4, 1]. Let's leave it for a separate since I moved it to MPB_wrapper
| if haskey(status_map, ret_val) | ||
| return new(x, y, s, status_map[ret_val], ret_val) |
There was a problem hiding this comment.
its better to use get(status_map, ret_val, :UnknownError) here
| return String(chars) | ||
| s = collect(info.status) | ||
| len = findfirst(iszero, s) - 1 | ||
| # There is no method String(::Vector{Cchar}) so we convert to `UInt8`. |
There was a problem hiding this comment.
you could String(reinterpret(UInt8, s)[1:len]) as well
There was a problem hiding this comment.
If we use the fact that UInt8 has the same binary representation of Cchar, we might as well store it as UInt8 in the struct
There was a problem hiding this comment.
yes, this is why my intial suggestion was to go with Cuchar; but this is a matter of naming/taste, so it can stay as it is now
(but this convert via array comprehension makes me wince :))
There was a problem hiding this comment.
Yes but I would rather have the code not depend on equivalence of bit representation just to have raw_status be slightly faster. What's wrong with array comprehension :-P ? How would you write it?
There was a problem hiding this comment.
I'm not feeling strongly about it; that bike-shedding was intended as a joke ;-).
I'd probably go for reinterpret(UInt8, s[1:len]) if we really do care about this one allocation (we don't:).
Edit: hmmm or maybe we do: s = rand(Cchar, 10); UInt8[s[i] for i in 1:5] results in ERROR: InexactError ... ??
Edit: sure, but scs should not put negative chars in status...
|
@blegat after suggested changes in |
Fix method ambiguity of MOIU.load_constraint
|
|
|
Not sure sure what you suggest, do you want to merge your branch before and tag v0.6 ? |
|
I thought You're about to merge this soon; scs-2.1.0 breaks binary compatibility and we have to modify SCSSettings struct accordingly (kalmarek@2c3c4c0), so that alone would be a reason to bump to v0.6; gpu-solver is a more extensive one: kalmarek@5855610 but if you don't object, I'd prepare pulls and merge these before |
Yes, this PR might be merged in a few days but you should have time to do it before. Just make sure to drop Julia v0.6 and v0.7. |
| after_success: | ||
| - julia -e 'cd(Pkg.dir("SCS")); Pkg.add("Coverage"); using Coverage; Coveralls.submit(Coveralls.process_folder())' | ||
| - julia -e 'import Pkg; Pkg.add("Coverage"); using Coverage; Coveralls.submit(process_folder())' |
There was a problem hiding this comment.
I believe that a simple - coveralls: true should do, according to docs:
https://docs.travis-ci.com/user/languages/julia/#coverage
There was a problem hiding this comment.
It worked on MOI and JuMP but it gives a weird error here:
https://travis-ci.org/JuliaOpt/SCS.jl/jobs/571513843#L936-L978
Maybe it works with codecov but not with coveralls
This reverts commit 0f4b33b.
We can still see the outcome, but GitHub does not consider it a failed test. I think this makes sense, since Julia 1.2 is not released yet?
Allow CI failure with Julia 1.2
Also drop Julia v0.6, Julia v0.7 and transition from REQUIRE to Project.toml.