Port performance updates for parameter broadcasting#95
Open
acostarelli wants to merge 1 commit intomainfrom
Open
Port performance updates for parameter broadcasting#95acostarelli wants to merge 1 commit intomainfrom
acostarelli wants to merge 1 commit intomainfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR ports performance-oriented updates for parameter broadcasting/indexing (referencing Sienna-Platform/PowerSimulations.jl#1609), aiming to reduce allocations and speed up parameter writes by improving index expansion and introducing integer-indexed “fast path” setters.
Changes:
- Reworked
expand_ixsto usentuple-based padding (compile-timeValpath forAbstractArray{...,N}plus a runtimendimsfallback). - Added specialized fast paths for scalar writes (
setindex!/JuMP.fix) when index tuples are fully specified. - Introduced
.dataaccessors onParameterContainerand added new internal integer-indexed setter helpers (_set_parameter_at!,_set_multiplier_at!,_set_param_value_at!).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/utils/indexing.jl |
Refactors index expansion and adds new specialized assignment/fix fast paths for DenseAxisArray-backed parameter writes. |
src/core/parameter_container.jl |
Adds .data accessors and introduces internal integer-index setter utilities intended to bypass DenseAxisArray axis-key lookup overhead. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+23
to
+30
| # Concrete fast path: scalar `src` with a fully-specified index tuple goes through `setindex!`. | ||
| @inline function assign_maybe_broadcast!( | ||
| dest::DenseAxisArray{T, N}, | ||
| src::T, | ||
| ixs::NTuple{N, Any}, | ||
| ) where {T, N} | ||
| dest[ixs...] = src | ||
| return |
| # Scalar/tuple `src`: broadcast the value across the indexed slice of `dest`. | ||
| @inline function assign_maybe_broadcast!(dest, src, ixs::Tuple) | ||
| expanded = expand_ixs(ixs, dest) | ||
| @views dest[expanded...] .= Ref(src) |
Comment on lines
+111
to
+116
| # Underlying dense storage of the parameter array. `parent` on a JuMP `DenseAxisArray` | ||
| # returns the array itself, so reach for `.data` directly to bypass the axis-keyed lookup. | ||
| get_parameter_array_data(c::ParameterContainer) = get_parameter_array(c).data | ||
| get_multiplier_array(c::ParameterContainer) = c.multiplier_array | ||
| # Same shortcut for the multiplier array — used by the integer-indexed fast path. | ||
| get_multiplier_array_data(c::ParameterContainer) = get_multiplier_array(c).data |
Comment on lines
+290
to
+306
| # Fast-path setters that skip DenseAxisArray's string-keyed axis lookup. Callers pass | ||
| # `get_parameter_array_data(container)` once, then write into the underlying Array | ||
| # by integer indices. The (i, t) layout matches the canonical (component, time) axis | ||
| # order produced by `add_param_container!`. | ||
| # | ||
| # 2D scalar path: covers Float64 and Tuple{Vararg{Float64}} eltypes (the latter is | ||
| # used by piecewise-cost MarketBid parameters whose storage is a Matrix of tuples). | ||
| @inline function _set_parameter_at!( | ||
| parent_param::Array{T, 2}, | ||
| ::JuMP.Model, | ||
| value::T, | ||
| i::Int, | ||
| t::Int, | ||
| ) where {T <: ValidDataParamEltypes} | ||
| parent_param[i, t] = value | ||
| return | ||
| end |
| src::Float64, | ||
| ixs::NTuple{N, Any}, | ||
| ) where {N} | ||
| JuMP.fix(dest[ixs...], src; force = true) |
Contributor
|
Performance Results This branch |
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.
Sienna-Platform/PowerSimulations.jl#1609