GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Update Nyquist rescaling + other improvements by murrayrm · Pull Request #1155 · python-control/python-control Conversation
Member
This PR addresses issue #1143 by changing the way that rescaling is handled in Using the example that @JonHowMIT raised in issue #1143, we go from this: to this: The original behavior can be retained by setting A few other changes that are also part of this PR:
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
|
There was an error while loading. Please reload this page.
|
coverage: 94.747% (-0.01%) from 94.759% |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| 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.
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() | |
| 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| 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.
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| 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.
The reason will be displayed to describe this comment to others. Learn more.
blend_curve_start is never None, so the first clause (blend_curve_start is None) can be deleted.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks for the careful review @slivingston. Incorporated your comments and will merge as soon as unit tests pass. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
Nyquist plot rescaling is confusing
There was an error while loading. Please reload this page.