Skip to content
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.

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

Check for unused subsystem signals in InterconnectedSystem #652

Merged

Conversation

roryyorke
Copy link
Contributor

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 ?

Add capability to check for unused signals in InterconnectedSystem;
this check is invoked by default by `interconnect`.
Ignore warnings with match string from conftest.py's `matrixfilter`
warning filter.
@coveralls
Copy link

coveralls commented Sep 11, 2021

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

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

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

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants