jump-dev / MatrixOptInterface.jl

An interface to pass matrix form problems
MIT License
11 stars 4 forks source link

Change Vector to AbstracVector in template type #4

Closed frapac closed 4 years ago

frapac commented 4 years ago

See #3: defining a LP problem with CuArrays could help integrating MatOI inside Simplex.jl

codecov[bot] commented 4 years ago

Codecov Report

Merging #4 into master will not change coverage. The diff coverage is 63.15%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master       #4   +/-   ##
=======================================
  Coverage   86.14%   86.14%           
=======================================
  Files           3        3           
  Lines         166      166           
=======================================
  Hits          143      143           
  Misses         23       23           
Impacted Files Coverage Δ
src/matrix_input.jl 98.52% <ø> (ø)
src/change_form.jl 73.97% <63.15%> (ø)

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 a5373c5...49bf9af. Read the comment docs.

frapac commented 4 years ago

I have added a new for loop to test sparse vectors as well. Let me know if you want me to add tests for other types of vectors.

matbesancon commented 4 years ago

One good way to test array genericity is Test.GenericArray which is meant exactly for that

julia> Test.GenericArray{Float64, 1}(1:4)
frapac commented 4 years ago

Thanks for the reference to Tests.GenericArray, I did not know this one. I will update the tests accordingly.

frapac commented 4 years ago

It was interesting to test MatOI with GenericArray. The tests break if we pass the vectors b and c as GenericArray, as we cannot convert Array{Float64, 1} to GenericArray{Float64, 1} automatically (on the contrary of CuArray or SparseVector).

For instance, when we use the function change_form as in https://github.com/jump-dev/MatrixOptInterface.jl/blob/master/src/change_form.jl#L20 we create a Array{Float64, 1} array with the function fill, and we currently expect the constructor to automatically convert it to the correct type. I think instead of using fill, we should create by default an array with the correct type (GenericArray here).

matbesancon commented 4 years ago

I see, array construction has no generic interface, only their use is

frapac commented 4 years ago

In that case, how about merging this PR and update the change_form function in another PR (I could open an issue accordingly)?

matbesancon commented 4 years ago

Yes I would also be for this solution

On Tue, Aug 18, 2020, 15:01 François Pacaud notifications@github.com wrote:

In that case, how about merging this PR and update the change_form function in another PR (I could open an issue accordingly)?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/jump-dev/MatrixOptInterface.jl/pull/4#issuecomment-675464342, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB2FDMVAYBFTZTNZV4HWMVDSBJ3TVANCNFSM4PGCLUNA .