Skip to content
This repository has been archived by the owner on Jun 14, 2020. It is now read-only.

Change the Implimentation of CSRMatrix to us SparseMatrixCSC #101

Merged
merged 2 commits into from
Apr 12, 2019
Merged

Change the Implimentation of CSRMatrix to us SparseMatrixCSC #101

merged 2 commits into from
Apr 12, 2019

Conversation

ndinsmore
Copy link
Contributor

This is a continuation of #96. Which got accidentally closed.
The gist of this is that it changes CSRMatrix to be an alias for Transpose{SparseMatrixCSC}

@codecov-io
Copy link

codecov-io commented Apr 3, 2019

Codecov Report

Merging #101 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #101      +/-   ##
==========================================
+ Coverage   88.25%   88.29%   +0.03%     
==========================================
  Files          12       12              
  Lines        1337     1341       +4     
==========================================
+ Hits         1180     1184       +4     
  Misses        157      157
Impacted Files Coverage Δ
src/constraints/vectorofvariables.jl 96.66% <ø> (ø) ⬆️
src/constraints/vectoraffine.jl 93.68% <100%> (+0.06%) ⬆️
src/constraints/scalaraffine.jl 94.4% <100%> (+0.03%) ⬆️
src/constraints.jl 93.87% <100%> (+0.54%) ⬆️
src/mockoptimizer.jl 80.66% <100%> (-0.1%) ⬇️

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 4769deb...11faadd. Read the comment docs.

@joaquimg
Copy link
Member

joaquimg commented Apr 4, 2019

So there is a great improvement on Clp, right? Have you benchmarked any other solvers?

@ndinsmore
Copy link
Contributor Author

This change it's self isn't really responsible for much performance improvement, it is more of a move towards base julia functionality. But it does result in on due to SparseMatrixCSC haveing many performance optimizations.

The benchmarks for this change were neutral because it effectively is just code simplification.

I do have the changes ready for Gurobi.jl, and GLPK.jl. Both of which are literally a few characters. I was just waiting for this to get merged before submitting them.

src/constraints.jl Outdated Show resolved Hide resolved
@ndinsmore
Copy link
Contributor Author

Are there any blocking issues or concerns from the reviewers?

@joaquimg
Copy link
Member

joaquimg commented Apr 9, 2019

If there are no regressions on gurobi I am ok with merging.
It would be great to see some benchmark though.

@ndinsmore
Copy link
Contributor Author

@joaquimg I don't have access to gurobi, but GLPK is similar in use and the benchmarks in jump-dev/GLPK.jl#95 show no change in performance.

@joaquimg
Copy link
Member

Then its good to go IMO.

@odow
Copy link
Member

odow commented Apr 11, 2019

Can we rebase this on the latest master please? Then it's good to merge.

ndinsmore and others added 2 commits April 11, 2019 15:37
…l value at the end of row_pointers that signifies the end of that row

Remove Depreciation  add sparse conversion

Small Fix

Change CSRMatrix to Adjoint{Sparse}}

Cleanup

Cleanup comments

Comment cleanup

cleanup

Changed Adjoint to Transpose

Rebase to no CI for julia v 0.6 (#1)

* Update appveyor.yml

remove testing for v0.6 add v1.1

* Update .travis.yml

drop testing for v0.6 add v1.1

* change require to julia 0.7

* This changes the implementation of CSRMatrix to include the additional value at the end of row_pointers that signifies the end of that row

* Remove Depreciation  add sparse conversion

* Small Fix

* Change CSRMatrix to Adjoint{Sparse}}

* Cleanup

* Cleanup comments

* Comment cleanup

* cleanup

* Changed Adjoint to Transpose

Remove commented lines
Fix Collect statement
drop v0.7
@ndinsmore
Copy link
Contributor Author

I think that we are good to go.

@odow odow merged commit 148334e into JuliaOpt:master Apr 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants