Perf T3-1: Remove unnecessary Num conversions#133
Open
hmzh-khn wants to merge 4 commits into
Open
Conversation
Tests verify that KKT condition vectors and parameter vectors are already Symbolics.Num-typed, making explicit Num.() conversion redundant. Includes test for _ensure_num_vec helper (not yet implemented). Co-Authored-By: Claude Opus 4.6 <[email protected]>
Add _ensure_num_vec helper that returns Vector{Num} unchanged (no-op) and
only converts non-Num vectors. Applied at lines 462 and 472 in
preoptimize_nonlinear_solver where vcat already produces Vector{Num}.
Impact: eliminates unnecessary broadcast allocation during solver setup.
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Add test_num_conversion.jl to expected file lists in test_test_tiers.jl so the tier self-test passes. All 543 fast tests now pass. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
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
2 participants
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
Remove redundant
Symbolics.Num()broadcast conversions inpreoptimize_nonlinear_solver(lines 462, 472 ofsrc/nonlinear_kkt.jl). Thevcatof symbolic vectors already producesVector{Num}, making theNum.()wrapper a no-op allocation.Adds a
_ensure_num_vechelper with two-method dispatch: returnsVector{Num}unchanged (identity), converts other vector types via broadcast.Changes
src/nonlinear_kkt.jl: Add_ensure_num_vechelper; replaceSymbolics.Num.(...)calls at MCP construction sitestest/test_num_conversion.jl: New test file verifying types are already Num, helper dispatch works, and MCP output is identicaltest/test_tiers.jl: Add new test to fast tiertest/test_test_tiers.jl: Update tier validation expected listsImpact
Negligible — this optimizes solver setup (one-time cost), not the solve hot path. Eliminates one unnecessary broadcast allocation per call to
preoptimize_nonlinear_solver.Testing
FAST_TESTS_ONLY=true)Changelog
🤖 Generated with Claude Code