GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Improvements to Nichols chart plotting by roryyorke · Pull Request #723 · python-control/python-control

Conversation

@roryyorke

@roryyorke roryyorke commented Apr 16, 2022

Copy link
Copy Markdown
Contributor

These are relatively minor updates to the Nichols chart (AKA grid) plotting.

It's far from ideal; further possible improvements:

  • more tightly fitting the chart to the view (or data) xlim and ylim (the "extent"); the pain here is figuring out which contours to plot, and where to place the labels
    • depending on the limits, some contours will end up with a two visible segments
  • recreating the axis on zoom and pan events
    • this might not be too difficult; Axes support 'xlim_changed' and 'ylim_changed' events
  • a deluxe version of this would be too add (secondary?) axes, and show the closed-loop gain and phase in the "data value" text (the "x= , y = " on the Matplotlib toolbar)
  • I wonder if we could use a custom ContourSets, and plug into however that labelling is done?

Here are some before-and-after pictures (left is before) of a few example systems.

The new code gives a better result in almost all respects. One visible problem is that the closed-loop phase label for "0°" clashes with the "-40dB" (and similar) closed-loop gain label. The gains become crowded when the gain span is large (final figure), but that is the same in both versions.

I haven't added tests; I'll see how much time I have to do that. Easyish tests:

  • check for "tight" phase limits (see first example below)
  • check that the axes child text elements are have clip_on (I'd have to not check title & tick labels, presume this is possible)
  • check for existence of phase labels, contingent on label_cl_phases flag
  • test both branches of the if in _inner_extents, and in fact test the behaviour in general by setting xlim & ylim before calling nichols_grid.
    Please suggest other tests.

example1
example2
example3
example4

Details (commit message)

Clip closed-loop contour labels.

Add labels for constant closed-loop phase contours.

Use smaller of data or view extents when deciding on how big, in
phase, a chart to create.

Use more uniformly spaced closed-loop phase contours, and use more
widely-spaced contours when phase extent is large.

Add optional ax argument, for axes to add grid to.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@roryyorke
Improvements to Nichols chart plotting
782e526
Clip closed-loop contour labels.

Add labels for constant closed-loop phase contours.

Use smaller of data or view extents when deciding on how big, in
phase, a chart to create.

Use more uniformly spaced closed-loop phase contours, and use more
widely-spaced contours when phase extent is large.

Add optional `ax` argument, for axes to add grid to.
@coveralls

coveralls commented Apr 16, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.1%) to 94.31% when pulling 59cd872 on roryyorke:rory/nichols-improvements into 2102181 on python-control:master.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@murrayrm

murrayrm commented Apr 25, 2022

Copy link
Copy Markdown
Member

Changes look good. It might be worth adding some unit tests to cover the new features (eg, use of ax keyword).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@roryyorke

roryyorke commented Apr 25, 2022

Copy link
Copy Markdown
Contributor Author

OK, I'll look at some tests, including testing the ax argument.

I'll probably wrap up the linfnorm PR first, and I'm not sure when I'll get this PR done.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

roryyorke added 3 commits April 27, 2022 10:05
@roryyorke
nichols_grid returns artists of grid elements; further tighten phase …
3281a8d
…extent
@roryyorke
Add tests for nichols_grid
b813e0c
@roryyorke
Fix nichols_grid test
71f3b61
@roryyorke

roryyorke commented Apr 27, 2022

Copy link
Copy Markdown
Contributor Author

I changed nichols_grid to return the artists it adds; this made testing much easier, but also makes it possible to customize the grid appearance and labels. Below is a not-especially-compelling example; more realistically one might change the intensity or alpha of the grid and labels.

plt.clf()
mc,nc,ml,nl = ct.nichols_grid()
for i,(c,l) in enumerate(zip(nc,nl)):
     nc.set_color(f'C{i}')
     nl.set_color(f'C{i}')
for i,(c,l) in enumerate(zip(mc,ml)):
     c.set_color(f'C{i}')
     l.set_color(f'C{i}')

color-nichols

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bnavigator
bnavigator reviewed Apr 27, 2022
View reviewed changes
Comment thread control/tests/nichols_test.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@roryyorke
In nichols_grid tests: use mplcleanup, and remove plt.clf calls
59cd872
@murrayrm

murrayrm commented Apr 30, 2022

Copy link
Copy Markdown
Member

This looks good to go to me. @roryyorke: any additional changes that you plan to make?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@roryyorke

roryyorke commented Apr 30, 2022 via email

Copy link
Copy Markdown
Contributor Author
No, all done.
…
On Sat, 30 Apr 2022, 07:05 Richard Murray, ***@***.***> wrote: This looks good to go to me. @roryyorke <https://github.com/roryyorke>: any additional changes that you plan to make? — Reply to this email directly, view it on GitHub <#723 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA3C7TMVNROIIOFAL7I2EDVHS5QRANCNFSM5TSN3NBA> . You are receiving this because you were mentioned.Message ID: ***@***.***>
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bnavigator
bnavigator merged commit ae8d586 into python-control:master Apr 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnavigator bnavigator bnavigator left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@roryyorke @coveralls @murrayrm @bnavigator
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.