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

murrayrm · 2024-11-09T05:13:21Z

This PR is motivated by discussion #1053 in which a situation the current implementation of find_eqpt does not find a good solution due to the problem being over/underconstrained. The main functional change is this PR is to help address that by adding a root_method method that allows a least squares solution to be obtained.

In addition, while looking through this (fairly old) code, it seemed worth making some additional changes:

Renamed find_eqpt to find_operating_point to be more consistent with python-control naming conventions (and the fact that an operating point is not always an equilibrium point.
Changed the return type for find_operating_point from a tuple of various lengths (depending on parameter settings) to a OperatingPoint object, which contains the states, inputs, and outputs corresponding to the operating point, along with the result of the call to the scipy.minimize.root function.
Added root_method and root_kwargs parameters to allow customization of the root finding method.
Maintained backward compatibility so that find_eqpt still works, as well as return_y (now return_outputs, with legacy processing of return_y) and return_result. In addition, the return object can be accessed as a tuple for consistency with the previous return signature.
Updated docstrings and documentation to use the new conventions.
Added unit tests to check all new functionality.

In the context of #1053, a solution to the issue there can be obtained by calling find_eqpt (now find_operating_point) with root_method='lm'.

coveralls · 2024-11-09T05:29:22Z

coverage: 94.692% (-0.002%) from 94.694%
when pulling e6fdc17 on murrayrm:find_operating_point-06Nov2024
into cc0020a on python-control:main.

slivingston

Good improvements! I made several comments to consider before merging.

I ran the example that originally motivated this pull request, and I confirmed the result is as expected after adding the new argument root_method='lm'.

Note that other files in examples/ besides cruise-control.py use find_eqpt(). However, it is now an alias for find_operating_point() without any indication of being deprecated, so OK to leave the other examples for now.

slivingston · 2024-11-10T02:16:52Z

control/tests/iosys_test.py

+    np.testing.assert_allclose(linsys_orig.C, linsys_oppt.C)
+    np.testing.assert_allclose(linsys_orig.D, linsys_oppt.D)
+
+    # Call find_operating point with method and keyword arguments


Suggested change

# Call find_operating point with method and keyword arguments

# Call find_operating_point with method and keyword arguments

slivingston · 2024-11-10T02:21:18Z

control/nlsys.py

@@ -92,7 +93,7 @@ class NonlinearIOSystem(InputOutputSystem):
        generic name <sys[id]> is generated with a unique integer id.

    params : dict, optional
-        Parameter values for the system.  Passed to the evaluation functions
+       Parameter values for the system.  Passed to the evaluation functions


The indentation step size is 4 spaces, but this change is to 3 spaces. Was this an accident?

slivingston · 2024-11-10T02:32:59Z

control/nlsys.py

+
+    """
+    def __init__(
+            self, states, inputs=None, outputs=None, result=None,


Suggested change

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

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

inputs parameter is not optional.

slivingston · 2024-11-10T18:36:55Z

control/tests/iosys_test.py

@@ -2087,6 +2088,101 @@ def test_find_eqpt(x0, ix, u0, iu, y0, iy, dx0, idx, dt, x_expect, u_expect):
    np.testing.assert_allclose(np.array(ueq), u_expect, atol=1e-6)


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


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

slivingston · 2024-11-10T20:06:01Z

control/nlsys.py

-    return_y : bool, optional
-        If True, return the value of output at the equilibrium point.
+        fixed at `dx0` in searching for an operating point.
+    root_method : str, optonal


Suggested change

root_method : str, optonal

root_method : str, optional

slivingston · 2024-11-10T21:03:04Z

control/nlsys.py

+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_outputs=None, return_result=None, **kwargs):


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.

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.

slivingston · 2024-11-10T21:26:38Z

control/nlsys.py

-              iu=None, iy=None, ix=None, idx=None, dx0=None,
-              return_y=False, return_result=False):
-    """Find the equilibrium point for an input/output system.
+class OperatingPoint(object):


Python 3 has classes inherit from object by default, so writing explicitly is not required: https://docs.python.org/3/reference/compound_stmts.html#class-definitions
However, I am not aware of a style recommendation. black does not add (object) and does not remove it, but the standard Python tutorial gives examples without explicit object: https://docs.python.org/3/tutorial/classes.html#class-objects

I forgot to make this comment on a recent pull request that I reviewed where a new class was defined. Anyway, we can keep explicit object inheritance for consistency with existing code and consider removing it later to be more concise.

Since we are using Python 3 and that only supports new classes, I think we should move to not inheriting from object (which is how things are done in the official documentation).

slivingston · 2024-11-10T22:14:51Z

control/nlsys.py

+        self.inputs = inputs
+
+        if outputs is None and return_outputs and not return_result:
+            raise SystemError("return_outputs specified by no y0 value")


Suggested change

raise SystemError("return_outputs specified by no y0 value")

raise SystemError("return_outputs specified but no y0 value")

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.

slivingston · 2024-11-10T22:15:34Z

control/nlsys.py

+        self.return_outputs = return_outputs
+
+        if result is None and return_result:
+            raise SystemError("return_result specified by no result value")


Suggested change

raise SystemError("return_result specified by no result value")

raise SystemError("return_result specified but no result value")

slivingston · 2024-11-10T22:36:10Z

examples/cruise-control.py

@@ -347,7 +347,7 @@ def cruise_plot(sys, t, y, label=None, t_hill=None, vref=20, antiwindup=False,

 # Compute the equilibrium throttle setting for the desired speed (solve for x
 # and u given the gear, slope, and desired output velocity)
-X0, U0, Y0 = ct.find_eqpt(
+X0, U0, Y0 = ct.find_operating_point(
    cruise_pi, [vref[0], 0], [vref[0], gear[0], theta0[0]],
    y0=[0, vref[0]], iu=[1, 2], iy=[1], return_y=True)


Suggested change

y0=[0, vref[0]], iu=[1, 2], iy=[1], return_y=True)

y0=[0, vref[0]], iu=[1, 2], iy=[1], return_outputs=True)

Update the argument to avoid a FutureWarning being raised in the example.

Similarly, return_y=True in examples/cruise.ipynb should be updated, too.

murrayrm added 6 commits November 6, 2024 08:23

create find_operating_point with updated documentation

7479353

add OperatingPoint object + unit test

fa162ec

allow OperatingPoint for linearize + code and documentation tweaks

223acc6

updated OperatingPoint attribute names + docstring edits

5eaee66

clean up docstrings, examples

aed5706

return_y -> return_outputs + related updates

1a2770c

slivingston self-requested a review November 10, 2024 02:13

slivingston reviewed Nov 10, 2024

View reviewed changes

murrayrm added 2 commits November 10, 2024 19:00

fix small typos in documentation (unrelated to main PR)

47e1c09

updated per @slivingston review comments

e6fdc17

murrayrm merged commit d810d5a into python-control:main Nov 14, 2024
23 checks passed

murrayrm added this to the 0.10.2 milestone Feb 19, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

murrayrm commented Nov 9, 2024

coveralls commented Nov 9, 2024 •

edited

Loading

slivingston left a comment

slivingston Nov 10, 2024

slivingston Nov 10, 2024

slivingston Nov 10, 2024

slivingston Nov 10, 2024

slivingston Nov 10, 2024

slivingston Nov 10, 2024

murrayrm Nov 11, 2024

slivingston Nov 10, 2024

murrayrm Nov 11, 2024

slivingston Nov 10, 2024

slivingston Nov 10, 2024

slivingston Nov 10, 2024

slivingston Nov 10, 2024

slivingston Nov 10, 2024

	# Call find_operating point with method and keyword arguments
	# Call find_operating_point with method and keyword arguments

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

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

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

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

	y0=[0, vref[0]], iu=[1, 2], iy=[1], return_y=True)
	y0=[0, vref[0]], iu=[1, 2], iy=[1], return_outputs=True)

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

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

Conversation

murrayrm commented Nov 9, 2024

coveralls commented Nov 9, 2024 • edited Loading

slivingston left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 9, 2024 •

edited

Loading