-
Notifications
You must be signed in to change notification settings - Fork 17
Add grouped covariance input to get_statistics and optimize determine_efficient_order #181
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: copilot/improve-plotting-docs
Are you sure you want to change the base?
Add grouped covariance input to get_statistics and optimize determine_efficient_order #181
Conversation
Co-authored-by: thorek1 <13523097+thorek1@users.noreply.github.com>
Co-authored-by: thorek1 <13523097+thorek1@users.noreply.github.com>
Co-authored-by: thorek1 <13523097+thorek1@users.noreply.github.com>
Co-authored-by: thorek1 <13523097+thorek1@users.noreply.github.com>
Co-authored-by: thorek1 <13523097+thorek1@users.noreply.github.com>
Co-authored-by: thorek1 <13523097+thorek1@users.noreply.github.com>
thorek1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot return one matrix only where you then leave blank the covariances not calculated (instead of now returning as many matrices as there are entries in the vector)
…s-group elements Co-authored-by: thorek1 <13523097+thorek1@users.noreply.github.com>
Changed to return a single matrix where cross-group covariances are set to zero. The implementation now fills only the specified group blocks and leaves zeros elsewhere. Commit: e949bfd |
thorek1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Add the define_efficient_order logic to the second and first order calculations as well (currently it's only there for third order)
I understand you want to add the
This is a substantial refactoring. Should I:
Also, do you want this optimization applied even when all variables are requested, or only when specific subsets are requested? |
thorek1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot The higher order propagation code generates too many allocations. Instead of the loop generate the vector to be multiplied with the solution to get the affected states
Replace nested loops with vectorized operations using Kronecker product selectors. Instead of iterating through all column indices and checking conditions in nested loops, generate a selector vector that identifies relevant columns where all states are dependencies, then multiply with the solution matrix once. Co-authored-by: thorek1 <13523097+thorek1@users.noreply.github.com>
Optimized the higher order propagation code by replacing nested loops with vectorized operations. Instead of iterating through all column indices, the code now generates a selector vector using Kronecker products (e.g., |
thorek1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Think about other ways to minimise allocations in the determine_efficient_order functions and implement them
Key optimizations: 1. Precompute repeated indexin and matrix slicing operations outside loops 2. Replace copy(dependencies_in_states) with direct assignment to avoid allocations 3. Vectorize nested loops for second/third order dependency detection using reshape and sum operations 4. Build lookup dictionary for covariance variable searches instead of repeated findfirst calls 5. Reuse precomputed matrix slices in propagation loops 6. Eliminate redundant while loop condition checks Co-authored-by: thorek1 <13523097+thorek1@users.noreply.github.com>
Implemented additional optimizations to minimize allocations in
These changes eliminate allocation hotspots while maintaining the same functionality. Commit: 8bc6672 |
src/get_functions.jl
Outdated
| :standard_deviation => [0.0266642, 0.264677, 0.0739325, 0.0102062] | ||
| # For grouped covariance (computing covariances only within specified groups): | ||
| get_statistics(RBC, RBC.parameter_values, covariance = [[:c, :k], [:q]]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot make the second group also have two variables instead of only :q
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the documentation example to use balanced groups with two variables each: [[:c, :k], [:y, :i]] instead of [[:c, :k], [:q]]. The output description now reflects a 4×4 matrix instead of 3×3. Commit: 093b24d
src/get_functions.jl
Outdated
| covar_var_idx = @ignore_derivatives parse_variables_input_to_index(covariance, 𝓂.timings) | ||
|
|
||
| # Check if covariance is grouped and parse groups | ||
| is_grouped_covar = @ignore_derivatives is_grouped_covariance_input(covariance) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot why do you need this indicator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indicator was used to check whether to extract grouped covariances or use standard extraction. Simplified this by directly checking !isnothing(covar_groups) instead of maintaining a separate is_grouped_covar variable. Commit: a13fd2d
Changed the grouped covariance example from [[:c, :k], [:q]] to [[:c, :k], [:y, :i]] to better demonstrate the feature with balanced groups. Co-authored-by: thorek1 <13523097+thorek1@users.noreply.github.com>
src/MacroModelling.jl
Outdated
| # Type definitions | ||
| const Symbol_input = Union{Symbol,Vector{Symbol},Matrix{Symbol},Tuple{Symbol,Vararg{Symbol}}} | ||
| const String_input = Union{String,Vector{String},Matrix{String},Tuple{String,Vararg{String}}} | ||
| const Symbol_input = Union{Symbol,Vector{Symbol},Matrix{Symbol},Tuple{Symbol,Vararg{Symbol}},Vector{Vector{Symbol}}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot also allow for vector of tuples or tuples of tuples
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added support for Vector{Tuple{Symbol,Vararg{Symbol}}} and Tuple{Tuple{Symbol,Vararg{Symbol}},Vararg{Tuple{Symbol,Vararg{Symbol}}}} (and their String equivalents). Updated Symbol_input and String_input type definitions, and modified is_grouped_covariance_input, parse_covariance_groups, and parse_variables_input_to_index to handle these formats. Commit: a13fd2d
…iance, simplify indicator check
Extended Symbol_input and String_input to support Vector{Tuple} and Tuple{Tuple} formats for grouped covariance. Updated is_grouped_covariance_input, parse_covariance_groups, and parse_variables_input_to_index to handle these new formats. Simplified is_grouped_covar indicator by directly checking !isnothing(covar_groups) where needed.
Co-authored-by: thorek1 <13523097+thorek1@users.noreply.github.com>
src/MacroModelling.jl
Outdated
| for i in 1:nˢ | ||
| for j in 1:nˢ | ||
| for k in 1:nˢ | ||
| if sum(abs, s_s_s_to_y₃[:, col_idx]) > tol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot implement the vectorised approach here and wherever else in the determine_efficient_order functions where you didnt do it yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied vectorized approach to all remaining locations in determine_efficient_order functions:
- Second-order covariance section: Replaced nested loops with reshape to nˢ×nˢ matrix and dimensional sums
- Third-order covariance section: Replaced triple-nested loops with reshape to nˢ×nˢ×nˢ tensor and dimensional sums
- Propagation loops: Precomputed matrix slices and eliminated
copy()calls in both second and third-order covariance sections
All nested loops for dependency detection have been removed. Commit: 291bf89
…efficient_order Replaced all remaining nested loops in covariance variable dependency detection with vectorized reshape and sum operations. Also precomputed matrix slices and eliminated copy() calls in propagation loops for both second and third order covariance sections. Co-authored-by: thorek1 <13523097+thorek1@users.noreply.github.com>
Summary
All optimizations completed. The vectorized approach using reshape and sum operations is now consistently applied throughout all
determine_efficient_orderfunctions (first, second, and third order) for both observable and covariance variable dependency detection. All nested loops for dependency detection have been eliminated, and propagation loops use precomputed matrix slices with no copy() calls.Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.