Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the profiling API to support passing backend-specific keyword arguments separately from the final plot-assembly keyword arguments, and adds tests to validate correct forwarding behavior.
Changes:
- Add
bp_kwargstoprofile_solversand forward it toBenchmarkProfiles.performance_profilecalls. - Update
performance_profiledocs/signature and explicitly callBenchmarkProfiles.performance_profile. - Add a new test (
profiles_kwargs.jl) and include it in the test runner.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/profiles.jl |
Introduces bp_kwargs forwarding to backend profile generation and updates docs/signatures. |
test/profiles_kwargs.jl |
Adds coverage to ensure backend kwargs vs plot kwargs are forwarded to the correct calls. |
test/runtests.jl |
Includes the new test file in the test suite. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function performance_profile( | ||
| stats::AbstractDict{Symbol, DataFrame}, | ||
| cost::Function, | ||
| args...; | ||
| cost::Function; | ||
| b::BenchmarkProfiles.AbstractBackend = PlotsBackend(), | ||
| kwargs..., | ||
| ) | ||
| solvers = keys(stats) | ||
| dfs = (stats[s] for s in solvers) | ||
| P = hcat([cost(df) for df in dfs]...) | ||
| performance_profile(b, P, string.(solvers), args...; kwargs...) | ||
| BenchmarkProfiles.performance_profile(b, P, string.(solvers); kwargs...) |
There was a problem hiding this comment.
The exported performance_profile wrapper previously accepted extra positional arguments (args...) and forwarded them to BenchmarkProfiles.performance_profile, but this method signature now removes that capability. Since this is part of the public API, this is a breaking change for any callers passing additional positional parameters; consider keeping args... (and forwarding them) or providing a deprecation path instead of removing it outright.
| width::Int = 400, | ||
| height::Int = 400, | ||
| b::BenchmarkProfiles.AbstractBackend = PlotsBackend(), | ||
| bp_kwargs::Dict = Dict(), | ||
| kwargs..., |
There was a problem hiding this comment.
bp_kwargs is typed as Dict with a default of Dict(), but keyword-splatting requires keys that can become keyword names (typically Symbol). As written, callers can pass non-Symbol keys and get a runtime error when splatting, and the Dict type annotation also blocks convenient alternatives like NamedTuple/Pairs which are commonly used for kwargs. Consider typing this as AbstractDict{Symbol,<:Any} (or accepting NamedTuple/Pairs as well) and/or validating/coercing keys before splatting.
| BenchmarkProfiles.performance_profile( | ||
| b, | ||
| Ps[1], | ||
| string.(solvers), | ||
| string.(solvers); | ||
| palette = colors, | ||
| title = costnames[1], | ||
| legend = :bottomright, | ||
| bp_kwargs..., | ||
| ), |
There was a problem hiding this comment.
bp_kwargs... is appended after explicit keywords like palette, title, and legend. If bp_kwargs contains any of those keys, the call will error due to repeated keyword arguments, making it impossible to override the defaults via bp_kwargs. Consider constructing a single keyword set (e.g., merge defaults with bp_kwargs giving bp_kwargs precedence, or explicitly rejecting conflicting keys with a clear error) to avoid runtime keyword-collision failures.
|
@arnavk23 Copilot has some good suggestions. |
|
@dpo I like the last 2 suggestions they are subtle suggestions to prevent further error breaking. In context of the first breaking change |
supersedes #189