Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 1, 2025

The plotting code for plot_irf and plot_irf! was duplicated across ~500 lines. This refactoring consolidates the logic into a single container function with unified plotting logic that handles both single and multiple IRF plots.

Changes

  • Added _plot_irf_from_container: Single consolidated function with unified plotting loop that handles both simple and comparison/stacking cases
  • Refactored plot_irf: Removed ~100 lines of inline plotting code, now delegates to _plot_irf_from_container
  • Refactored plot_irf!: Removed ~460 lines of inline plotting code, now delegates to _plot_irf_from_container
  • Unified plotting logic: Removed separate code paths for single vs multiple containers; single loop structure now handles both cases with embedded conditional checks

Impact

  • Eliminates ~640 lines of duplicated/separate plotting code (net reduction of ~80 lines)
  • Single source of truth for all IRF plotting logic
  • Changes to IRF plotting logic now made in one place
  • Unified loop structure reduces maintenance burden
  • No API changes

Implementation

The consolidated _plot_irf_from_container function uses a unified plotting approach:

  • Difference detection varies by container length (minimal for single, full comparison for multiple)
  • Unified loop structure: Always builds joint_shocks and joint_variables, iterates over them regardless of container count
  • Embedded conditionals: Only where behavior actually differs (subplot type, legend/annotation inclusion)
  • Single plotting flow handles both 1 container and N containers seamlessly

Key difference from typical container patterns: Instead of separate code branches for single vs multiple cases, the logic is unified with minimal conditional checks embedded only where needed.

Example

# Simple case: plot_irf populates container, calls _plot_irf_from_container
plot_irf(model, periods = 20)

# Comparison case: plot_irf! appends to container, calls _plot_irf_from_container
plot_irf(model, algorithm = :first_order)
plot_irf!(model, algorithm = :second_order, plot_type = :compare)

Both cases now use the same plotting loop internally, with conditional logic only where behavior differs.

Original prompt

write a plot_irf_container function in a similar way as was done for plot_solution. so that in the end the plotting code for plot_irf and plot_irf! lives in a single function


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 3 commits November 1, 2025 11:25
Co-authored-by: thorek1 <13523097+thorek1@users.noreply.github.com>
Co-authored-by: thorek1 <13523097+thorek1@users.noreply.github.com>
Copilot AI changed the title [WIP] Add plot_irf_container function for unified IRF plotting Consolidate plot_irf plotting logic into container function Nov 1, 2025
Copilot AI requested a review from thorek1 November 1, 2025 11:33
Copilot finished work on behalf of thorek1 November 1, 2025 11:33
Copy link
Owner

@thorek1 thorek1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot There should be only
One irf container plotting function combining the logic from plot irf and plot irf!

Copy link
Owner

@thorek1 thorek1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot There should be only one irf container plotting function combining the logic from plot_irf and plot_irf!

Co-authored-by: thorek1 <13523097+thorek1@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 1, 2025

@copilot There should be only one irf container plotting function combining the logic from plot_irf and plot_irf!

Done! I've consolidated the plotting logic into a single _plot_irf_from_container function that combines logic from both plot_irf and plot_irf! using conditional logic (if/else), following the exact same pattern as _plot_solution_from_container.

Changes in commit 3e5aae1:

  • Created ONE consolidated container function with both simple and comparison logic inline
  • Removed the separate helper functions (_plot_irf_simple_from_container and _plot_irf_comparison_from_container)
  • Both plot_irf and plot_irf! now call the single container function

The structure now matches plot_solution exactly - one main container function that checks if length == 1 for simple case, else for comparison case.

Copilot AI changed the title Consolidate plot_irf plotting logic into container function Consolidate plot_irf plotting logic into single container function Nov 1, 2025
Copilot AI requested a review from thorek1 November 1, 2025 16:21
Copilot finished work on behalf of thorek1 November 1, 2025 16:21
Copy link
Owner

@thorek1 thorek1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot combjne the logic for a single irf plot and multiple irf plots instead of doing if else on the length of the container

Co-authored-by: thorek1 <13523097+thorek1@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 1, 2025

@copilot combjne the logic for a single irf plot and multiple irf plots instead of doing if else on the length of the container

Done! I've unified the plotting logic by removing the main if/else branching on container length.

Changes in commit 05a52f1:

  • Removed the large if length == 1 vs else structure (~210 lines of duplicated code)
  • Created a unified plotting loop that handles both single and multiple containers
  • Embedded conditional checks only where behavior actually differs (subplot type, legend inclusion)
  • The core plotting flow is now in one place, making it easier to maintain

The structure now:

  1. Difference detection varies by length (efficient for single case)
  2. Unified loop always builds joint_shocks/variables and iterates
  3. Conditionals only for subplot call (simple vs comparison) and plot assembly (with/without annotations)

Copilot AI changed the title Consolidate plot_irf plotting logic into single container function Consolidate plot_irf plotting logic into unified container function Nov 1, 2025
Copilot AI requested a review from thorek1 November 1, 2025 23:03
Copilot finished work on behalf of thorek1 November 1, 2025 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants