combine plots #127
Open
combine plots #127
Conversation
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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #127 +/- ##
==========================================
- Coverage 82.43% 74.29% -8.15%
==========================================
Files 18 19 +1
Lines 10625 11923 +1298
==========================================
+ Hits 8759 8858 +99
- Misses 1866 3065 +1199 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Benchmark Results
Benchmark PlotsA plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR. |
…data visualization
There was a problem hiding this comment.
Pull Request Overview
This PR adds combined plotting functionality to MacroModelling.jl that allows users to layer multiple economic model results on the same plots. The main addition is a suite of "bang" functions (ending with !) that can be called sequentially to add additional series to existing plots, enabling side-by-side comparison of different parameter configurations, models, or shock scenarios.
Key changes include:
- Implementation of combined plotting functions (
plot_irf!,plot_model_estimates!, etc.) - Centralization of default options and common plotting logic
- Addition of
get_model_estimatesfunction that combines estimated variables and shocks - Enhanced error handling and user feedback throughout the codebase
Reviewed Changes
Copilot reviewed 20 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_estimation.jl | Adds test for combined model estimates plotting |
| test/runtests.jl | Extensive test suite additions for combined plotting across multiple test sets |
| test/functionality_tests.jl | Updates functionality test to accept two models and adds comprehensive combined plotting tests |
| test/fix_combined_plots.jl | New comprehensive test file demonstrating combined plotting functionality |
| src/macros.jl | Improves warning messages for invalid macro options |
| src/get_functions.jl | Adds get_model_estimates function and updates default options usage |
| src/filter/inversion.jl | Minor error handling improvement |
| src/default_options.jl | New file centralizing all default option constants |
| src/common_docstrings.jl | Updates documentation strings to use centralized default values |
| src/MacroModelling.jl | Major updates including new plotting functions, helper utilities, and enhanced shock processing |
| ext/TuringExt.jl | Updates to use centralized default option constants |
| docs/src/unfinished_docs/todo.md | Adds new TODO items for future improvements |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const DEFAULT_SYLVESTER_ALGORITHM = :doubling | ||
| const DEFAULT_SYLVESTER_THRESHOLD = 1000 | ||
| const DEFAULT_LARGE_SYLVESTER_ALGORITHM = :bicgstab | ||
| const DEFAULT_SYLVESTER_SELECTOR = 𝓂 -> sum(1:𝓂.timings.nPast_not_future_and_mixed + 1 + 𝓂.timings.nExo) > DEFAULT_SYLVESTER_THRESHOLD ? DEFAULT_LARGE_SYLVESTER_ALGORITHM : DEFAULT_SYLVESTER_ALGORITHM |
There was a problem hiding this comment.
This complex selector function embedded as a constant makes the code harder to understand. Consider extracting this into a named function with a descriptive name like select_sylvester_algorithm(𝓂) for better readability and maintainability.
Suggested change
| const DEFAULT_SYLVESTER_SELECTOR = 𝓂 -> sum(1:𝓂.timings.nPast_not_future_and_mixed + 1 + 𝓂.timings.nExo) > DEFAULT_SYLVESTER_THRESHOLD ? DEFAULT_LARGE_SYLVESTER_ALGORITHM : DEFAULT_SYLVESTER_ALGORITHM | |
| function select_sylvester_algorithm(𝓂) | |
| total = sum(1:𝓂.timings.nPast_not_future_and_mixed + 1 + 𝓂.timings.nExo) | |
| if total > DEFAULT_SYLVESTER_THRESHOLD | |
| return DEFAULT_LARGE_SYLVESTER_ALGORITHM | |
| else | |
| return DEFAULT_SYLVESTER_ALGORITHM | |
| end | |
| end | |
| const DEFAULT_SYLVESTER_SELECTOR = select_sylvester_algorithm |
Copilot uses AI. Check for mistakes.
|
|
||
| import LoopVectorization: @turbo | ||
| import Polyester | ||
| # import Polyester |
There was a problem hiding this comment.
Commented-out import suggests incomplete refactoring. If Polyester is no longer needed, the line should be removed entirely. If it's temporarily disabled, add a comment explaining why.
Suggested change
| # import Polyester |
Copilot uses AI. Check for mistakes.
…quation_algorithm, sylvester_algorithm, and lyapunov_algorithm (#151) * Initial plan * Complete plotting_script.jl documentation for tol, quadratic_matrix_equation_algorithm, sylvester_algorithm, and lyapunov_algorithm Co-authored-by: thorek1 <13523097+thorek1@users.noreply.github.com> * Update plotting_script.jl --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: thorek1 <13523097+thorek1@users.noreply.github.com> Co-authored-by: Thore Kockerols <thorek1@users.noreply.github.com>
…al_forecast functions
…handling for tolerances and algorithms
…ine plotting functions
…ling.jl into combine_plots
…152) * Initial plan * Add plotting.md documentation and generate_plots.jl script Co-authored-by: thorek1 <13523097+thorek1@users.noreply.github.com> * Add plotting documentation references to docstrings Co-authored-by: thorek1 <13523097+thorek1@users.noreply.github.com> * Add README for plot generation and note to plotting_script.jl Co-authored-by: thorek1 <13523097+thorek1@users.noreply.github.com> * Remove plotting documentation references Removed references to comprehensive plotting documentation in multiple sections. * Remove SAVE_PLOTS_NAME constant Removed the SAVE_PLOTS_NAME constant from common docstrings. * Delete docs/README_plots.md * Fix plot filename references to match automatic naming convention Co-authored-by: thorek1 <13523097+thorek1@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: thorek1 <13523097+thorek1@users.noreply.github.com> Co-authored-by: Thore Kockerols <thorek1@users.noreply.github.com>
…onse function plots
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>
…ing impulse response function plots and saving them to the documentation assets directory. This script was previously used to facilitate the regeneration of documentation plots.
…on! for combining multiple solutions (#153) * Initial plan * Refactor plot_solution to accept single algorithm and add plot_solution! Co-authored-by: thorek1 <13523097+thorek1@users.noreply.github.com> * Update tests for new plot_solution API Co-authored-by: thorek1 <13523097+thorek1@users.noreply.github.com> * Remove duplicate include of default_options.jl in MacroModelling.jl * Add relevant input differences table to plot_solution! for comparing multiple solutions Co-authored-by: thorek1 <13523097+thorek1@users.noreply.github.com> * Fix plot_solution to handle single container case on first call Co-authored-by: thorek1 <13523097+thorek1@users.noreply.github.com> * Implement legend with 2 columns and show relevant input difference for single difference case Co-authored-by: thorek1 <13523097+thorek1@users.noreply.github.com> * Add support for multiple states with separate plot sets per state Co-authored-by: thorek1 <13523097+thorek1@users.noreply.github.com> * order of legend items * Adjust layout heights in _plot_solution_from_container to accommodate varying input differences * fix plot solution * Enhance functionality tests by adding comprehensive plot_solution! tests for multiple algorithms and states --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: thorek1 <13523097+thorek1@users.noreply.github.com> Co-authored-by: thorek1 <thorek1@users.noreply.github.com> Co-authored-by: Thore Kockerols <Thore.Kockerols@ecb.europa.eu>
…977-682bd45f4534
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
No description provided.