adds Dict{Symbol,DataFrame} as input for performance_profile #54

lrsantos11 · 2021-04-06T18:49:59Z

Implemented according to #48 and with SolverBenchmark.jl as metioned by @tmigot

codecov · 2021-04-06T18:54:59Z

Codecov Report

Merging #54 (3b6688f) into master (6717159) will decrease coverage by 3.63%.
The diff coverage is 45.45%.

@@            Coverage Diff             @@
##           master      #54      +/-   ##
==========================================
- Coverage   94.96%   91.33%   -3.64%     
==========================================
  Files           4        4              
  Lines         139      150      +11     
==========================================
+ Hits          132      137       +5     
- Misses          7       13       +6

Impacted Files	Coverage Δ
src/BenchmarkProfiles.jl	`100.00% <ø> (ø)`
src/performance_profiles.jl	`80.59% <45.45%> (-6.91%)`	⬇️

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 6717159...3b6688f. Read the comment docs.

test/runtests.jl

src/performance_profiles.jl

test/runtests.jl

Project.toml

src/BenchmarkProfiles.jl

Project.toml

dpo · 2021-04-07T21:30:38Z

Do you have a really old branch that GitHub Actions aren’t running?

lrsantos11 · 2021-04-07T21:52:24Z

I don't now what to say. Sorry for that. I guess I cannot have, because I'm in a new computer. First folder in ~/.julia/dev.

lrsantos11 · 2021-04-07T22:10:44Z

Is this related with some error onGR? See JuliaLang/julia#36617 (comment)

abelsiqueira · 2021-04-07T23:17:40Z

Cirrus has been failing for a while, can we just remove it? @dpo

@lrsantos11, can you rebase this, it has conflicting files now.

dpo · 2021-04-08T00:28:32Z

Cirrus fails because I think the GR jll isn’t yet available there. For the tests, I really think we should just depend on UnicodePlots or some such. In fact, now that we can export the profile data without producing a plot, I wonder if we should make Plots optional and use Require.jl.

abelsiqueira · 2021-04-08T01:16:54Z

Makes sense. Do you want to do that before this is merged?

dpo · 2021-04-09T15:14:46Z

Done in #58 (not fun to review).

dpo · 2021-04-13T13:44:04Z

@lrsantos11 would you mind rebasing this? It shouldn’t be too hard (famous last words).

lrsantos11 · 2021-04-13T14:41:24Z

@lrsantos11 would you mind rebasing this? It shouldn’t be too hard (famous last words).

I really think I did it in the last push. I asked @abelsiqueira but though I was done. Certanly not. Can you guys guide this newbee?

dpo · 2021-04-13T14:48:17Z

Yes but there have been new changes in master and now your PR has conflicts. You can do

git checkout master
git pull
git checkout dataframe
git rebase master
# now fix any conflicts

abelsiqueira · 2021-04-13T14:52:30Z

We had other changes in #58 to fix the error in the Cirrus CI, and this has conflicts again. I can fix it though, I have it open right now.

Co-authored-by: Dominique <dominique.orban@gmail.com>

abelsiqueira · 2021-04-13T20:08:14Z

Took longer than I hoped because of last-minute issues popping up for the classes of next semester, so naturally, something broke.

Co-authored-by: Abel Siqueira <nepper271@gmail.com>

Co-authored-by: Dominique <dominique.orban@gmail.com>

abelsiqueira · 2021-04-13T20:32:59Z

Ok, I've uncovered several things.

Plots only support Julia-1.3 up to version 1.6
Plots 1.6 has Reexport 0.2 as requirement
Some package requests Reexport 1.0, don't know which one, because DataFrames allows Reexport 0.2. But DataFrames has a lot of deps, so maybe one of those.
The only solution I thought of is to drop Julia 1.3 and move the bound to 1.5. You guys have other ideas?

abelsiqueira · 2021-04-13T21:17:10Z

I made a PR to this branch in your repo @lrsantos11. Instead of using Dict{Symbol, DataFrames}, your new method function would be based on a Dict{Symbol,<: Any} instead. @dpo what do you think?

lrsantos11 · 2021-04-13T21:25:55Z

I agree with @abelsiqueira. Just let me know @dpo if it is ok for you, then I can accept the PR and we can finally finish this.

dpo · 2021-04-13T21:45:15Z

Why not make DataFrames an optional dependency?

dpo · 2021-04-13T20:25:16Z

src/performance_profiles.jl

@@ -129,3 +129,26 @@ function performance_profile(b::AbstractBackend,
  (x_plot, y_plot, max_ratio) = performance_profile_data(T, logscale=logscale, sampletol=sampletol, drawtol=drawtol)
  performance_profile_plot(b, x_plot, y_plot, max_ratio, xlabel, ylabel, labels, title, logscale; kwargs...)
 end
+
+performance_profile(b::AbstractBackend, T :: Array{Tn,2}, labels :: Vector{S}; kwargs...) where {Tn <: Number, S <: AbstractString} =
+  performance_profile(b, convert(Array{Float64,2}, T), convert(Vector{AbstractString}, labels); kwargs...)


I removed this because it’s ambiguous. At some point I got a StackOverflow because of it. In addition, it’s not tested.

dpo · 2021-04-13T21:46:58Z

src/performance_profiles.jl

+Examples of cost functions:
+- `cost(df) = df.elapsed_time`: Simple `elapsed_time` cost. Assumes the solver solved the problem.
+- `cost(df) = (df.status .!= :first_order) * Inf + df.elapsed_time`: Takes the status of the solver into consideration.
+"""


Could we use the same docstring format as in the existing performance_profile?

abelsiqueira reviewed Apr 7, 2021

View reviewed changes

test/runtests.jl Outdated Show resolved Hide resolved

abelsiqueira reviewed Apr 7, 2021

View reviewed changes

test/runtests.jl Outdated Show resolved Hide resolved

abelsiqueira requested a review from dpo April 7, 2021 18:01

dpo requested changes Apr 7, 2021

View reviewed changes

dpo reviewed Apr 7, 2021

View reviewed changes

Project.toml Outdated Show resolved Hide resolved

lrsantos11 force-pushed the dataframe branch from c9d8936 to eb59757 Compare April 8, 2021 18:15

lrsantos11 added 2 commits April 13, 2021 12:15

adds Dict{Symbol,DataFrame} as input for performance_profile

77e2c48

corrects test of DataFrames

874ca95

abelsiqueira force-pushed the dataframe branch from eb59757 to a3eac70 Compare April 13, 2021 20:03

lrsantos11 and others added 5 commits April 13, 2021 17:03

Update src/performance_profiles.jl

5d60df4

Co-authored-by: Dominique <dominique.orban@gmail.com>

Update src/performance_profiles.jl

21cfc6d

Co-authored-by: Dominique <dominique.orban@gmail.com>

Update src/performance_profiles.jl

dea02ca

Co-authored-by: Dominique <dominique.orban@gmail.com>

Update src/performance_profiles.jl

0db073a

Co-authored-by: Dominique <dominique.orban@gmail.com>

Put DataFrames before Plots.

1082660

lrsantos11 added 3 commits April 13, 2021 17:08

Put DataFrames before Plots.

a3eac70

adds Dict{Symbol,DataFrame} as input for performance_profile

87064ff

corrects test of DataFrames

1b584c6

lrsantos11 and others added 4 commits April 13, 2021 17:18

Update test/runtests.jl

c0e4b01

Co-authored-by: Abel Siqueira <nepper271@gmail.com>

Update test/runtests.jl

361f7ab

Co-authored-by: Abel Siqueira <nepper271@gmail.com>

Update src/performance_profiles.jl

e9fea0e

Co-authored-by: Dominique <dominique.orban@gmail.com>

Update src/performance_profiles.jl

967d883

Co-authored-by: Dominique <dominique.orban@gmail.com>

Update src/performance_profiles.jl

3b6688f

abelsiqueira mentioned this pull request Apr 13, 2021

Remove DataFrames as a requirement and use as <: Any instead lrsantos11/BenchmarkProfiles.jl#1

Open

dpo reviewed Apr 13, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adds Dict{Symbol,DataFrame} as input for performance_profile #54

adds Dict{Symbol,DataFrame} as input for performance_profile #54

lrsantos11 commented Apr 6, 2021

codecov bot commented Apr 6, 2021 •

edited

Loading

dpo commented Apr 7, 2021

lrsantos11 commented Apr 7, 2021

lrsantos11 commented Apr 7, 2021 •

edited

Loading

abelsiqueira commented Apr 7, 2021

dpo commented Apr 8, 2021

abelsiqueira commented Apr 8, 2021

dpo commented Apr 9, 2021

dpo commented Apr 13, 2021

lrsantos11 commented Apr 13, 2021

dpo commented Apr 13, 2021

abelsiqueira commented Apr 13, 2021

abelsiqueira commented Apr 13, 2021

abelsiqueira commented Apr 13, 2021

abelsiqueira commented Apr 13, 2021

lrsantos11 commented Apr 13, 2021

dpo commented Apr 13, 2021

dpo Apr 13, 2021

dpo Apr 13, 2021

adds Dict{Symbol,DataFrame} as input for performance_profile #54

Are you sure you want to change the base?

adds Dict{Symbol,DataFrame} as input for performance_profile #54

Conversation

lrsantos11 commented Apr 6, 2021

codecov bot commented Apr 6, 2021 • edited Loading

Codecov Report

dpo commented Apr 7, 2021

lrsantos11 commented Apr 7, 2021

lrsantos11 commented Apr 7, 2021 • edited Loading

abelsiqueira commented Apr 7, 2021

dpo commented Apr 8, 2021

abelsiqueira commented Apr 8, 2021

dpo commented Apr 9, 2021

dpo commented Apr 13, 2021

lrsantos11 commented Apr 13, 2021

dpo commented Apr 13, 2021

abelsiqueira commented Apr 13, 2021

abelsiqueira commented Apr 13, 2021

abelsiqueira commented Apr 13, 2021

abelsiqueira commented Apr 13, 2021

lrsantos11 commented Apr 13, 2021

dpo commented Apr 13, 2021

dpo Apr 13, 2021

Choose a reason for hiding this comment

dpo Apr 13, 2021

Choose a reason for hiding this comment

codecov bot commented Apr 6, 2021 •

edited

Loading

lrsantos11 commented Apr 7, 2021 •

edited

Loading