GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — update model_reduction to allow input/output selection + unstable warning (vs exception) by murrayrm · Pull Request #1074 · python-control/python-control

Conversation

@murrayrm

@murrayrm murrayrm commented Dec 4, 2024

Copy link
Copy Markdown
Member

This PR updates the model_reduction function to add some additional functionality. From the docstring:

This function produces a reduced-order model of a system by eliminating specified inputs, outputs, and/or states from the original system. The specific states, inputs, or outputs that are eliminated can be specified by either listing the states, inputs, or outputs to be eliminated or those to be kept.

Specific updates:

  • Changed the ELIM positional parameter to elim_states (positional or keyword).
  • Added keep_states to allow specifying the list of states to keep instead of those to eliminate.
  • Added elim_inputs, elim_outputs, keep_inputs, keep_outputs to allow selection of inputs and outputs.
  • Specifications of states, inputs, and outputs can be done by a list of integers (original implementation) as well as slices or lists of signal names (new).
  • In the case of eliminating states, inputs, or outputs from an unstable system, a warning is issued (versus an exception) and this warning can be suppressed using warn_unstable=False.
  • Removed the copyright info from the top of modelsimp.py since we know just use the LICENSE file.
  • Add unit tests and documentation updates.
  • Cleaned up import order (isort -m2) and removed extraneous spaces.

This PR was motivated by an example from FBS2e (see steering.py), where I wanted to extract the dynamics of a subsystem (and the original system had an eigenvalue at the origin). It is possible to accomplish the same result by using the old implementation (assuming the exception is changed to a warning) and then indexing the system directly to handle the inputs and outputs, but it seemed natural to include in a single function call.

Sorry, something went wrong.

Uh oh!

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

All reactions
murrayrm added 7 commits November 29, 2024 08:23
@murrayrm
allow state elimination on unstable systems with warning
aad5502
@murrayrm
update unit tests
e6824d7
@murrayrm
initial refactoring of model_reduction (no changes in functionality)
77b25d5
@murrayrm
add input/output removal + unit tests
ae43212
@murrayrm
update documentation for basic functionality
310798e
@murrayrm
add slice processing
b96a039
@murrayrm
add signal name processing
412ef97
@coveralls

coveralls commented Dec 4, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown

Coverage Status

coverage: 94.756% (+0.07%) from 94.689%
when pulling eff6c40 on murrayrm:modred_unstable-28Nov2024
into 69efbbe on python-control:main.

All reactions

Sorry, something went wrong.

Uh oh!

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

@slivingston
slivingston self-requested a review December 5, 2024 17:06
slivingston
slivingston reviewed Dec 5, 2024
View reviewed changes

@slivingston 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 improvement! I made several stylistic and misprint-related comments. Else, ready to merge.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread control/modelsimp.py Outdated
eliminated or those to be kept.

Two methods of state reduction are possible: 'truncate' removes the
states marked for elimination, while 'matchdc' replaces the the

@slivingston slivingston Dec 5, 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
states marked for elimination, while 'matchdc' replaces the the
states marked for elimination, while 'matchdc' replaces the

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread control/modelsimp.py Outdated
Comment on lines +144 to +146
unstable eigenvalues, since in those situations the stability reduced
order model may be different that the stability of the full model. No
other checking is done, so users to be careful not to render a system

@slivingston slivingston Dec 5, 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
unstable eigenvalues, since in those situations the stability reduced
order model may be different that the stability of the full model. No
other checking is done, so users to be careful not to render a system
unstable eigenvalues, since in those situations the stability of the reduced
order model may be different than the stability of the full model. No
other checking is done, so users must be careful not to render a system

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread control/modelsimp.py Outdated
dico = 'C'
else:
raise NotImplementedError("Function not implemented in discrete time")
States, inputs, and outputs can be specified using integer offers or

@slivingston slivingston Dec 5, 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
States, inputs, and outputs can be specified using integer offers or
States, inputs, and outputs can be specified using integer offsets or

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread control/tests/modelsimp_test.py Outdated
({'elim_inputs': [0, 1, 2]}, 5, 3, 0), # no inputs
({'elim_outputs': [0, 1, 2]}, 5, 0, 3), # no outputs
({'elim_states': [0, 1, 2, 3, 4]}, 0, 3, 3), # no states
({'elim_states': [0, 1], 'keep_states': [1, 2]}, None, None, None)

@slivingston slivingston Dec 5, 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
({'elim_states': [0, 1], 'keep_states': [1, 2]}, None, None, None)
({'elim_states': [0, 1], 'keep_states': [1, 2]}, None, None, None),

Trailing comma because, then, adding items in the future will not cause a diff on this line, as recommended by PEP 8 and enforced by Black.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread control/modelsimp.py Outdated
See Also
--------
balanced_reduction : Eliminate states using Hankel singular values.
minimal_realization : Eliminate unreachable or unobseravble states.

@slivingston slivingston Dec 5, 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
minimal_realization : Eliminate unreachable or unobseravble states.
minimal_realization : Eliminate unreachable or unobservable states.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread control/modelsimp.py Outdated
warnings.warn("System is unstable; reduction may be meaningless")

# Utility function to process keep/elim keywords
def _process_elim_or_keep(elim, keep, labels, allow_both=False):

@slivingston slivingston Dec 5, 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.

Any reason to keep allow_both? _process_elim_or_keep() is a function defined only in the scope of model_reduction(), and all calls to it have the default allow_both value (allow_both=False).

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread control/modelsimp.py Outdated
method : string
Method of removing states in `ELIM`: either 'truncate' or
'matchdc'.
Method of removing states in `elim`: either 'truncate' or

@slivingston slivingston Dec 5, 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
Method of removing states in `elim`: either 'truncate' or
Method of removing inputs, outputs, and/or states: either 'truncate' or

This is confusing because there is no elim parameter and because inputs and outputs can be removed as well, in addition to states.

Sorry, something went wrong.

Uh oh!

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

All reactions

@murrayrm murrayrm Dec 7, 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.

Will update, but this only applies to the way states are removed.

Sorry, something went wrong.

Uh oh!

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

All reactions
slivingston
slivingston reviewed Dec 5, 2024
View reviewed changes
Comment thread control/modelsimp.py Outdated

"""
if not isinstance(sys, StateSpace):
raise TypeError("system must be a a StateSpace system")

@slivingston slivingston Dec 5, 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 TypeError("system must be a a StateSpace system")
raise TypeError("system must be a StateSpace system")

Sorry, something went wrong.

Uh oh!

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

All reactions
@murrayrm
updates to address @slivingston review comments
eff6c40
slivingston
slivingston approved these changes Dec 7, 2024
View reviewed changes
@murrayrm
murrayrm merged commit e2c0ff8 into python-control:main Dec 7, 2024
@murrayrm murrayrm added this to the 0.10.2 milestone Feb 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@slivingston slivingston slivingston approved these changes

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

@murrayrm @coveralls @slivingston
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.