Skip to content

Update find_eqpt to find_operating_point, adding root_method + better docs #1054

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 8 commits into from
Nov 14, 2024
Merged

Update find_eqpt to find_operating_point, adding root_method + better docs #1054

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7479353
create find_operating_point with updated documentation
murrayrm Nov 6, 2024
fa162ec
add OperatingPoint object + unit test
murrayrm Nov 6, 2024
223acc6
allow OperatingPoint for linearize + code and documentation tweaks
murrayrm Nov 7, 2024
5eaee66
updated OperatingPoint attribute names + docstring edits
murrayrm Nov 8, 2024
aed5706
clean up docstrings, examples
murrayrm Nov 8, 2024
1a2770c
return_y -> return_outputs + related updates
murrayrm Nov 9, 2024
47e1c09
fix small typos in documentation (unrelated to main PR)
murrayrm Nov 11, 2024
e6fdc17
updated per @slivingston review comments
murrayrm Nov 11, 2024
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
Prev Previous commit
Next Next commit
return_y -> return_outputs + related updates
  • Loading branch information
@murrayrm
murrayrm committed Nov 9, 2024
commit 1a2770c9c34295c5928d197f80e0fed85495aae5
40 changes: 23 additions & 17 deletions control/nlsys.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1694,14 +1694,14 @@ class OperatingPoint(object):
"""
def __init__(
self, states, inputs=None, outputs=None, result=None,

@slivingston slivingston Nov 10, 2024

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.

Suggested change
self, states, inputs=None, outputs=None, result=None,
self, states, inputs, outputs=None, result=None,

inputs parameter is not optional.

return_y=False, return_result=False):
return_outputs=False, return_result=False):
self.states = states
self.inputs = inputs

if outputs is None and return_y and not return_result:
raise SystemError("return_y specified by no y0 value")
if outputs is None and return_outputs and not return_result:
raise SystemError("return_outputs specified by no y0 value")

@slivingston slivingston Nov 10, 2024

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.

Suggested change
raise SystemError("return_outputs specified by no y0 value")
raise SystemError("return_outputs specified but no y0 value")

@slivingston slivingston Nov 10, 2024

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.

Here and below are both non-standard uses of SystemError, which normally is for "when the [Python] interpreter finds an internal error". I recommend ValueError instead, which is also used elsewhere in this module for inconsistent arguments.

self.outputs = outputs
self.return_y = return_y
self.return_outputs = return_outputs

if result is None and return_result:
raise SystemError("return_result specified by no result value")

@slivingston slivingston Nov 10, 2024

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.

Suggested change
raise SystemError("return_result specified by no result value")
raise SystemError("return_result specified but no result value")

Expand All @@ -1710,9 +1710,9 @@ def __init__(

# Implement iter to allow assigning to a tuple
def __iter__(self):
if self.return_y and self.return_result:
if self.return_outputs and self.return_result:
return iter((self.states, self.inputs, self.outputs, self.result))
elif self.return_y:
elif self.return_outputs:
return iter((self.states, self.inputs, self.outputs))
elif self.return_result:
return iter((self.states, self.inputs, self.result))
Expand All @@ -1729,9 +1729,9 @@ def __len__(self):


def find_operating_point(
sys, x0, u0=None, y0=None, t=0, params=None,
iu=None, iy=None, ix=None, idx=None, dx0=None, root_method=None,
root_kwargs=None, return_y=False, return_result=False):
sys, x0, u0=None, y0=None, t=0, params=None, iu=None, iy=None,
ix=None, idx=None, dx0=None, root_method=None, root_kwargs=None,
return_outputs=None, return_result=None, **kwargs):

@slivingston slivingston Nov 10, 2024

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.

Suggested change
return_outputs=None, return_result=None, **kwargs):
return_outputs=False, return_result=False, **kwargs):

return_outputs and return_result are not used in a way where the distinction between None and False matters. I recommend to write the default value in the expected type, i.e., False, because the docstring shows these as bool, optional without explicitly stating the default value, so users will look at the function signature and may be confused if it is None.

@murrayrm murrayrm Nov 11, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This needs to be None in order for the legacy keyword handling to work: if both return_y and return_outputs are specified, _process_legacy_keyword will raise an exception. Thus with this change, setting the default value of return_outputs to False would mean that if you call find_operating_point with return_y, you get an exception instead of a warning.

"""Find an operating point for an input/output system.

An operating point for a nonlinear system is a state and input around
Expand Down Expand Up @@ -1804,8 +1804,8 @@ def find_operating_point(
is passed to the :func:`scipy.optimize.root` function.
root_kwargs : dict, optional
Additional keyword arguments to pass :func:`scipy.optimize.root`.
return_y : bool, optional
If True, return the value of output at the operating point.
return_outputs : bool, optional
If True, return the value of outputs at the operating point.
return_result : bool, optional
If True, return the `result` option from the
:func:`scipy.optimize.root` function used to compute the
Expand All @@ -1820,8 +1820,8 @@ def find_operating_point(
:class:`OperatingPoint` for a description of other attributes.

If accessed as a tuple, returns `states`, `inputs`, and optionally
`outputs` and `result` based on the `return_y` and `return_result`
parameters.
`outputs` and `result` based on the `return_outputs` and
`return_result` parameters.

Notes
-----
Expand All @@ -1844,6 +1844,12 @@ def find_operating_point(
"""
from scipy.optimize import root

# Process keyword arguments
return_outputs = config._process_legacy_keyword(
kwargs, 'return_y', 'return_outputs', return_outputs)
if kwargs:
raise TypeError("unrecognized keyword(s): " + str(kwargs))

# Process arguments for the root function
root_kwargs = dict() if root_kwargs is None else root_kwargs
if root_method:
Expand Down Expand Up @@ -2019,14 +2025,14 @@ def rootfun(z):
# Return the result based on what the user wants and what we found
if return_result or result.success:
return OperatingPoint(
z[0], z[1], z[2], result, return_y, return_result)
z[0], z[1], z[2], result, return_outputs, return_result)
else:
# Something went wrong, don't return anything
return OperatingPoint(
None, None, None, result, return_y, return_result)
None, None, None, result, return_outputs, return_result)

# TODO: remove code when ready

@slivingston slivingston Nov 10, 2024

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.

The above if/else block returns always, so the code from # TODO and below is unreachable. In other words, the code is ready to be removed (as in the TODO comment).

if not return_y:
if not return_outputs:
z = z[0:2] # Strip y from result if not desired
if return_result:
# Return whatever we got, along with the result dictionary
Expand All @@ -2036,7 +2042,7 @@ def rootfun(z):
return z
else:
# Something went wrong, don't return anything
return (None, None, None) if return_y else (None, None)
return (None, None, None) if return_outputs else (None, None)


# Linearize an input/output system
Expand Down
1 change: 1 addition & 0 deletions control/tests/docstrings_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
control.create_estimator_iosystem: ['state_labels'], # deprecated
control.bode_plot: ['sharex', 'sharey', 'margin_info'], # deprecated
control.eigensys_realization: ['arg'], # quasi-positional
control.find_operating_point: ['method'], # internal use
}

# Decide on the level of verbosity (use -rP when running pytest)
Expand Down
40 changes: 38 additions & 2 deletions control/tests/iosys_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,11 @@

import re
import warnings
import pytest
from math import sqrt

import numpy as np
from math import sqrt
import pytest
import scipy

import control as ct

Expand Down Expand Up @@ -2088,6 +2089,7 @@ def test_find_eqpt(x0, ix, u0, iu, y0, iy, dx0, idx, dt, x_expect, u_expect):


# Test out new operating point version of find_eqpt
# TODO: add return_)y tests

@slivingston slivingston Nov 10, 2024

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.

There is a test below (function test_operating_point) that uses return_y. Is something else intended with this TODO?

def test_find_operating_point():
dt = 1
sys = ct.NonlinearIOSystem(
Expand Down Expand Up @@ -2122,6 +2124,10 @@ def test_find_operating_point():
assert op_point.inputs is not None
assert op_point.result.success is False

# Check to make sure unknown keywords are caught
with pytest.raises(TypeError, match="unrecognized keyword"):
ct.find_operating_point(sys, x0, u0, unknown=None)


def test_operating_point():
dt = 1
Expand All @@ -2146,6 +2152,36 @@ def test_operating_point():
op_point = ct.find_operating_point(
sys, 0, 0, root_method='lm', root_kwargs={'tol': 1e-6})

# Make sure we can get back the right arguments in a tuple
op_point = ct.find_operating_point(sys, 0, 0, return_outputs=True)
assert len(op_point) == 3
assert isinstance(op_point[0], np.ndarray)
assert isinstance(op_point[1], np.ndarray)
assert isinstance(op_point[2], np.ndarray)

with pytest.warns(FutureWarning, match="return_outputs"):
op_point = ct.find_operating_point(sys, 0, 0, return_y=True)
assert len(op_point) == 3
assert isinstance(op_point[0], np.ndarray)
assert isinstance(op_point[1], np.ndarray)
assert isinstance(op_point[2], np.ndarray)

# Make sure we can get back the right arguments in a tuple
op_point = ct.find_operating_point(sys, 0, 0, return_result=True)
assert len(op_point) == 3
assert isinstance(op_point[0], np.ndarray)
assert isinstance(op_point[1], np.ndarray)
assert isinstance(op_point[2], scipy.optimize.OptimizeResult)

# Make sure we can get back the right arguments in a tuple
op_point = ct.find_operating_point(
sys, 0, 0, return_result=True, return_outputs=True)
assert len(op_point) == 4
assert isinstance(op_point[0], np.ndarray)
assert isinstance(op_point[1], np.ndarray)
assert isinstance(op_point[2], np.ndarray)
assert isinstance(op_point[3], scipy.optimize.OptimizeResult)


def test_iosys_sample():
csys = ct.rss(2, 1, 1)
Expand Down
3 changes: 3 additions & 0 deletions control/tests/kwargs_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import control.tests.frd_test as frd_test
import control.tests.freqplot_test as freqplot_test
import control.tests.interconnect_test as interconnect_test
import control.tests.iosys_test as iosys_test
import control.tests.optimal_test as optimal_test
import control.tests.statefbk_test as statefbk_test
import control.tests.stochsys_test as stochsys_test
Expand Down Expand Up @@ -252,6 +253,8 @@ def test_response_plot_kwargs(data_fcn, plot_fcn, mimo):
'dlqr': test_unrecognized_kwargs,
'drss': test_unrecognized_kwargs,
'feedback': test_unrecognized_kwargs,
'find_eqpt': iosys_test.test_find_operating_point,
'find_operating_point': iosys_test.test_find_operating_point,
'flatsys.flatsys': test_unrecognized_kwargs,
'frd': frd_test.TestFRD.test_unrecognized_keyword,
'gangof4': test_matplotlib_kwargs,
Expand Down