Forward keyword arguments in profile_solvers - #208
Open
arnavk23 wants to merge 2 commits into
Open
Conversation
… profile_solvers Adds a `bp_kwargs` keyword to `profile_solvers` so backend-specific options such as `logscale` can be forwarded to each `BenchmarkProfiles.performance_profile` call, separately from the `kwargs` passed to the final `plot` call. Addresses review feedback from PR JuliaSmoothOptimizers#200: `bp_kwargs` accepts any `AbstractDict{Symbol}` (not just `Dict`), and its entries are merged with (and take precedence over) the internal `palette`/`title`/`legend` defaults instead of being splatted after them, avoiding duplicate-keyword errors. `performance_profile`'s public signature (including `args...`) is left untouched to avoid a breaking change. Fixes JuliaSmoothOptimizers#143 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The profile_solvers docstring currently overstates bp_kwargs precedence for title despite rotate = true later overwriting titles, which should be corrected to avoid misleading API documentation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates profile_solvers to accept a separate bp_kwargs keyword argument so backend-specific options can be forwarded to each BenchmarkProfiles.performance_profile call, while keeping kwargs... reserved for the final plot call that assembles the profile wall.
Changes:
- Added
bp_kwargskeyword toprofile_solversand merged it with internal defaults (palette,title,legend) so user-provided values take precedence without duplicate-keyword errors. - Updated internal calls to explicitly use
BenchmarkProfiles.performance_profileand pass merged keyword arguments via splatting. - Added tests to ensure
bp_kwargsis accepted and can override internal defaults without error.
File summaries
| File | Description |
|---|---|
src/profiles.jl |
Adds bp_kwargs to profile_solvers and forwards merged backend kwargs into each BenchmarkProfiles.performance_profile call. |
test/profiles.jl |
Adds coverage that bp_kwargs is accepted and that overriding defaults (e.g., legend, palette) does not error. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
arnavk23
changed the title
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #143.
profile_solversgains abp_kwargskeyword so backend-specific options (e.g.logscale) can be forwarded to eachBenchmarkProfiles.performance_profilecall, separately from thekwargspassed to the finalplotcall that assembles the profile wall.This reimplements the approach from #200 on top of current
main(which has gained therotateoption since #200 was opened) and addresses the review feedback left there by Copilot:performance_profile's public signature, including theargs...passthrough, is left untouched — no behavior removed.bp_kwargstyping: typed asAbstractDict{Symbol}(defaultDict{Symbol,Any}()) rather than a bareDict, so anySymbol-keyed dict works.bp_kwargsismerged with the internal defaults (palette,title,legend) instead of being splatted after them, sobp_kwargsentries take precedence and duplicate-keyword errors can't occur.Test plan
test/profiles.jlexercisingbp_kwargs(e.g.logscale = false) and confirming overriding internal defaults likelegend/paletteviabp_kwargsdoesn't error.Pkg.test()); all tests pass.🤖 Generated with Claude Code