- Notifications
You must be signed in to change notification settings - Fork441
Update Nyquist rescaling + other improvements#1155
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Update Nyquist rescaling + other improvements#1155
Uh oh!
There was an error while loading.Please reload this page.
Conversation
coveralls commentedJun 21, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
I made several code quality comments. In terms of clarity of plots, I think this new rescaling behavior is good and ready to merge.
control/tests/nyquist_test.py Outdated
| from datetime import date | ||
| # Create the file to store figures | ||
| git_info = subprocess.check_output(['git', 'describe'], text=True).strip() |
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.
| git_info=subprocess.check_output(['git','describe'],text=True).strip() | |
| try: | |
| git_info=subprocess.check_output(['git','describe'],text=True).strip() | |
| exceptsubprocess.CalledProcessError: | |
| git_info='UNKNOWN-REPO-INFO' |
This will fail if the tests are run from outside the Git repository directory, which is possible from anywhere using
python -m control.tests.nyquist_testTo handle this, I recommend to either default to something like "UNKNOWN" or to warn the user ("unable to get version info from git") and not include{git_info} in the file name.
control/freqplot.py Outdated
| if blend_fraction < 0 or blend_fraction > 1: | ||
| raise ValueError("blend_fraction must be between 0 and 1") | ||
| blend_curve_start = (1 - blend_fraction) * max_curve_magnitude |
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.
Neither checking feasibility ofblend_fraction nor declaringblend_curve_start depends on the loop variables (idx andresponse), so these can be moved to before the start of this for-loop.
control/freqplot.py Outdated
| abs_subset = np.abs(subset) | ||
| unit_vectors = subset / abs_subset # Preserve phase/direction | ||
| if blend_curve_start is None or \ |
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.
blend_curve_start is neverNone, so the first clause (blend_curve_start is None) can be deleted.
Thanks for the careful review@slivingston. Incorporated your comments and will merge as soon as unit tests pass. |
This PR addresses issue#1143 by changing the way that rescaling is handled in$\infty$ mapping from the blend point to
nyquist_plot. The prior code saturated the Nyquist curve atmax_curve_magnitude, which would leave to confusing plots when the Nyquist curve had interesting features at large magnitude, since all of that detail was "collapsed" onto the limiting circle. In the new code, a 1-1 rescaling function is used that maps large magnitude features into a continuous range of magnitudes starting at a "blend point" and going out tomax_curve_magnitude. The result is that the features of the Nyquist curve can be more easily visualized. The fraction of the Nyquist curve that is "blended" is set with a new parameterblend_fraction, that defaults to 0.20 (so the last 20% of the Nyquist curve prior tomax_curve_magnitudeis rescaled, with everything from the blend point tomax_curve_magnitude.Using the example that@JonHowMIT raised in issue#1143, we go from this:
to this:
The original behavior can be retained by setting
blend_fraction=0.A few other changes that are also part of this PR:
max_curve_magnitudefrom a default of 20 to 15 (this allows the -1 point to be see better).control/tests/nyquist_test.control/tests/nyquist_testto generate a gallery of plots when run as a script (via python or ipython).nyquist_plotto match its documentation. In particular,cplt.lineswas returning a 1D array instead of a 2D array (as done in all other_plotfunctions).