Skip to content

Turn off warn_infinite for phase_crossover_frequencies() calculation #1106

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
murrayrm merged 2 commits into from
Jan 30, 2025
Merged

Turn off warn_infinite for phase_crossover_frequencies() calculation #1106

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6fcf741
turn off warn_infinite for phase_crossover_frequencies() calculation
murrayrm Jan 29, 2025
d9e44d2
address @slivingston review comments
murrayrm Jan 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
turn off warn_infinite for phase_crossover_frequencies() calculation
  • Loading branch information
@murrayrm
murrayrm committed Jan 29, 2025
commit 6fcf7419e7b30148a8e7a287ee6790a2806cbaf0
13 changes: 9 additions & 4 deletions control/margins.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,11 +81,16 @@ def _poly_iw_sqr(pol_iw):

def _poly_iw_real_crossing(num_iw, den_iw, epsw):
# Return w where imag(H(iw)) == 0

# Compute the imaginary part of H = (num.r + j num.i)/(den.r + j den.i)
test_w = np.polysub(np.polymul(num_iw.imag, den_iw.real),
np.polymul(num_iw.real, den_iw.imag))

# Find the real-valued w > 0 where imag(H(iw)) = 0
w = np.roots(test_w)
w = np.real(w[np.isreal(w)])
w = w[w >= epsw]

return w


Expand Down Expand Up @@ -471,7 +476,7 @@ def phase_crossover_frequencies(sys):
omega : ndarray
1d array of (non-negative) frequencies where Nyquist plot
intersects the real axis
gain : ndarray
gains : ndarray
1d array of corresponding gains

Examples
Expand All @@ -493,13 +498,13 @@ def phase_crossover_frequencies(sys):
omega = _poly_iw_real_crossing(num_iw, den_iw, 0.)

# using real() to avoid rounding errors and results like 1+0j
gain = np.real(evalfr(sys, 1J * omega))
gains = np.real(sys(omega * 1j, warn_infinite=False))
else:
zargs = _poly_z_invz(sys)
z, omega = _poly_z_real_crossing(*zargs, epsw=0.)
gain = np.real(evalfr(sys, z))
gains = np.real(sys(z, warn_infinite=False))

return omega, gain
return omega, gains


def margin(*args):
Expand Down
13 changes: 7 additions & 6 deletions control/tests/margin_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,11 @@
from numpy import inf, nan
from numpy.testing import assert_allclose

from control.frdata import FrequencyResponseData
from control.margins import (margin, phase_crossover_frequencies,
stability_margins)
from control.statesp import StateSpace
from control.xferfcn import TransferFunction
from control.exception import ControlMIMONotImplemented
from ..frdata import FrequencyResponseData
from ..margins import margin, phase_crossover_frequencies, stability_margins
from ..statesp import StateSpace
from ..xferfcn import TransferFunction
from ..exception import ControlMIMONotImplemented

@slivingston slivingston Jan 29, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use relative imports in test files?

Arguments against relative imports in test files

  • Although the tests/ directory is a subpackage and therefore can use the relative import syntax, this change will prevent running tests from outside of the control package.
  • Users of control necessarily write absolute imports (import control or from control...). Tests of the public API should follow the patterns of users.

Arguments in favor

  • Relative import syntax is more concise.
  • I might have missed something, but none of the CI jobs run the tests against a copy of control other than where they are located. In other words, "running tests from outside of the control package" is not done in practice, so not important to support.
  • We are already using the relative import syntax in config_test.py.

For comparison, neither numpy nor scipy seem to be consistent: most of their tests are written using absolute imports (import numpy or import scipy etc.), but some tests use relative imports.

Conclusion

I like the generality in principle of keeping these as absolute imports, but practically both patterns seem good.


s = TransferFunction.s

Expand Down Expand Up @@ -119,6 +118,8 @@ def test_margin_3input(tsys):
(([2], [1, 3, 3, 1]), [1.732, 0.], [-0.25, 2.]),
((np.array([3, 11, 3]) * 1e-4, [1., -2.7145, 2.4562, -0.7408], .1),
[1.6235, 0.], [-0.28598, 1.88889]),
(([200.0], [1.0, 21.0, 20.0, 0.0]),
[4.47213595, 0], [-0.47619048, inf]),

@slivingston slivingston Jan 29, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warnings do not cause the test to fail, so this case does not provide a regression test. To fix this, add the decorator @pytest.mark.filterwarnings("error") as done in another test in margin_test.py.

])
def test_phase_crossover_frequencies(tfargs, omega_ref, gain_ref):
"""Test phase_crossover_frequencies() function"""
Expand Down