Skip to content

Check for unused subsystem signals in InterconnectedSystem#652

Merged
sawyerbfuller merged 3 commits intopython-control:masterfrom
roryyorke:rory/check-interconnect-signals
Nov 6, 2021
Merged

Check for unused subsystem signals in InterconnectedSystem#652
sawyerbfuller merged 3 commits intopython-control:masterfrom
roryyorke:rory/check-interconnect-signals

Conversation

@roryyorke
Copy link
Contributor

@roryyorke roryyorke commented Sep 11, 2021

Add capability to check for unused signals in InterconnectedSystem;
this check is invoked by default by interconnect.

This would have saved me a few hours when I made three typos in signal names, and I couldn't figure out why my time and frequency responses were all wrong.

Some questions:

  • is check_unused=True a reasonable default? I think it is; it won't take long to scan the unused signals, and, if necessary, add them to the ignore_* lists
  • are the ignore_* names reasonable? I initially called them dropped_inputs and dropped_outputs, but I decided against that. known_unused_inputs is more explicit, but a bit long.
  • should this be in the InterconnectedSystem constructor, rather than in interconnect ?

@roryyorke
Check for unused subsystem signals in InterconnectedSystem
78b3349 
Add capability to check for unused signals in InterconnectedSystem;
this check is invoked by default by `interconnect`.
@roryyorke
Handle matrix warnings in test_interconnect_unused_{input,output}
2224ea5 
Ignore warnings with match string from conftest.py's `matrixfilter`
warning filter.
@coveralls
Copy link

coveralls commented Sep 11, 2021 •
edited
Loading

Coverage Status

Coverage increased (+0.08%) to 90.006% when pulling faf3145 on roryyorke:rory/check-interconnect-signals into 5ab0a1c on python-control:master.

@murrayrm
Copy link
Member

murrayrm commented Sep 11, 2021

Quick responses on a few things, based on a quick read:

  • I really like this functionality and completely agree we should include it!

  • I think it would be OK to put this in the interconnect() function rather than the InterconnectedSystem constructor. The intent of interconnect() was to provide some additional processing and a more user-friendly interface.

  • It looks like there are some glitches in the documentation (ignore_inputs appears twice).

@roryyorke
Copy link
Contributor Author

roryyorke commented Sep 12, 2021

Thanks for the feedback. I've fixed the docstring.

think it would be OK to put this in the interconnect()

OK, good. If InterconnectedSystem is used directly, check_unused_signals is available to be called after mappings have been set up. I also made unused_signals public, though I don't know what applications it could have.

While writing this, it occurred to me it wouldn't be all that hard to output a Graphviz dot file, or, possibly, a data structure that could be fed into NetworkX, to allow respectively visualization and graph-theoretic analysis of the signal flow, but I don't have time for any of that at the moment.

@sawyerbfuller
Copy link
Contributor

sawyerbfuller commented Sep 14, 2021

Connecting @petercorke into this discussion because he is working on a nice-looking graphical renderer for this kind of interconnection. Not sure it is released yet though.

@petercorke
Copy link

petercorke commented Sep 15, 2021 via email

@sawyerbfuller sawyerbfuller merged commit ce20b24 into python-control:master Nov 6, 2021
@roryyorke roryyorke deleted the rory/check-interconnect-signals branch November 8, 2021 18:04
@murrayrm murrayrm added this to the 0.9.1 milestone Dec 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.9.1

Development

Successfully merging this pull request may close these issues.

5 participants

@roryyorke @coveralls @murrayrm @sawyerbfuller @petercorke

Comments