GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Update find_eqpt to find_operating_point, adding root_method + better docs by murrayrm · Pull Request #1054 · python-control/python-control

Conversation

murrayrm commented Nov 9, 2024

Copy link

Copy Markdown

Member

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'.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

murrayrm added 6 commits

November 6, 2024 08:23


          create find_operating_point with updated documentation


          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

coveralls commented Nov 9, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

slivingston self-requested a review

November 10, 2024 02:13

slivingston reviewed

Nov 10, 2024

View reviewed changes

slivingston left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

control/tests/iosys_test.py Outdated

		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

slivingston Nov 10, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

control/nlsys.py Outdated


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

slivingston Nov 10, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

control/nlsys.py Outdated


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

slivingston Nov 10, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

control/tests/iosys_test.py Outdated



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

slivingston Nov 10, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

control/nlsys.py Outdated

		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

slivingston Nov 10, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change

	root_method : str, optonal
	root_method : str, optional

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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):

slivingston Nov 10, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

murrayrm Nov 11, 2024

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

control/nlsys.py Outdated

		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):

slivingston Nov 10, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

murrayrm Nov 11, 2024

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

control/nlsys.py Outdated

		self.inputs = inputs

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

slivingston Nov 10, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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")

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

slivingston Nov 10, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

control/nlsys.py Outdated

		self.return_outputs = return_outputs

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

slivingston Nov 10, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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")

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

examples/cruise-control.py Outdated

		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)

slivingston Nov 10, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

slivingston Nov 10, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

murrayrm added this to the 0.10.2 milestone

Feb 19, 2025

Reviewers

slivingston

slivingston left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.10.2

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

murrayrm commented Nov 9, 2024

Uh oh!

coveralls commented Nov 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slivingston left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented Nov 9, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.