Update Nyquist rescaling + other improvements #1155
Conversation
slivingston
left a comment
There was a problem hiding this comment.
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.
| git_info = subprocess.check_output(['git', 'describe'], text=True).strip() | |
| try: | |
| git_info = subprocess.check_output(['git', 'describe'], text=True).strip() | |
| except subprocess.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_test
To 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.
Neither checking feasibility of blend_fraction nor declaring blend_curve_start depends on the loop variables (idx and response), 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.
blend_curve_start is never None, 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).