Skip to content

Fix marginplot variable assignment for >5 phase margins#1056

Merged
baggepinnen merged 1 commit into
masterfrom
fix/marginplot-copypaste
May 16, 2026
Merged

Fix marginplot variable assignment for >5 phase margins#1056
baggepinnen merged 1 commit into
masterfrom
fix/marginplot-copypaste

Conversation

@baggepinnen
Copy link
Copy Markdown
Member

@baggepinnen baggepinnen commented May 15, 2026

✔️

Summary

When a system has more than 5 phase margins, the code that filters to the 5 smallest assigned the filtered values to wgm/gm (the gain-margin display variables) instead of wpm/pm. This silently overwrote gain-margin data with phase-margin data on the plot.

Likely copy-paste from the analogous block immediately above (which correctly filters gain margins).

Test plan

  • Plot marginplot for a system with more than 5 phase margins and verify the gain-margin annotations are now correct.
  • Existing test_plots.jl still passes.

🤖 Generated with Claude Code

The block that limits the displayed phase margins to the 5 smallest
mistakenly assigned to `wgm`/`gm` (the gain-margin display variables)
instead of `wpm`/`pm`, overwriting the gain-margin data with phase-margin
data whenever `length(pm) > 5`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@JuliaControlBot
Copy link
Copy Markdown

This is an automated message.
Plots were compared to references. 4/11 images have changed, see differences below.
After pulling this PR, please update the reference images by creating a PR to ControlExamplePlots.jl here.

Difference Reference Image New Image
✔️ 0.0 Reference New
✔️ 0.0 Reference New
✔️ 0.0 Reference New
✔️ 0.0 Reference New

@JuliaControlBot
Copy link
Copy Markdown

This is an automated message.
Plots were compared to references. 4/11 images have changed, see differences below.
After pulling this PR, please update the reference images by creating a PR to ControlExamplePlots.jl here.

Difference Reference Image New Image
✔️ 0.0 Reference New
✔️ 0.0 Reference New
✔️ 0.0 Reference New
✔️ 0.0 Reference New

@codecov
Copy link
Copy Markdown

codecov Bot commented May 15, 2026

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.61%. Comparing base (0aa6e4c) to head (da64b63).

Files with missing lines Patch % Lines
lib/ControlSystemsBase/src/plotting.jl 0.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1056   +/-   ##
=======================================
  Coverage   91.61%   91.61%           
=======================================
  Files          42       42           
  Lines        5653     5653           
=======================================
  Hits         5179     5179           
  Misses        474      474           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@baggepinnen baggepinnen merged commit 973e205 into master May 16, 2026
6 of 7 checks passed
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