Skip to content

new iosys.interconnect is great, but... #530

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.

Sign up for GitHub

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

Jump to bottom
Closed
sawyerbfuller opened this issue Feb 3, 2021 · 3 comments · Fixed by #536
Closed

new iosys.interconnect is great, but... #530

sawyerbfuller opened this issue Feb 3, 2021 · 3 comments · Fixed by #536
Assignees
@murrayrm

Comments

@sawyerbfuller
Copy link
Contributor

sawyerbfuller commented Feb 3, 2021 •
edited
Loading

The new naming functionality in iosys.interconnect function [1] has allowed me to create some very sophisticated systems much more easily than with append and connect. So far, no bugs that I have found. Very nice!

a few remarks on usage:

  • the inplist and outlist arguments are confusing and tricky. When you are writing code, its easy to forget shortened argument names like this: was it outplist or outlist? Also, they seem to exist to serve as a means to provide new names to the overall inputs and outputs. Is that a necessary feature? (might have to do with how system was set up for explicit interconnection) Possible to remove them and force the overall system to have the same input names and output names as constituent systems, and instead just provide inputs and outputs kwargs for interconnect? (edit: this is what matlab seems to do: https://www.mathworks.com/help/control/ref/connect.html)
  • auto-summing and auto-splitting, as suggested here: https://github.com/python-control/python-control/blob/master/doc/iosys.rst#automatic-connections-using-signal-names doesn't seem to work. For example, when I try to send the same signal u to both sys1 and sys2 in the following code a ValueError: signal u is not unique is raised:
sys1 = ct.tf(1, [10, 1])
sys2 = ct.tf(1, [2, 1])
summer = ct.iosys.summing_junction(['y1', 'y2'], 'y')
sys1 = ct.iosys.tf2io(sys1, inputs='u', outputs='y1')
sys2 = ct.iosys.tf2io(sys2, inputs='u', outputs='y2')
parallel = ct.interconnect((summer, sys1, sys2), inplist='u', outlist='y')
  • similarly for auto-summing. the following raises ValueError: signal y is not unique:
splitter = ct.ss2io(ct.ss([], [], [], [[1],[1]]), inputs='u', outputs=('u1', 'u2'))
sys1 = ct.iosys.tf2io(sys1, inputs='u1', outputs='y')
sys2 = ct.iosys.tf2io(sys2, inputs='u2', outputs='y')
parallel = ct.interconnect((splitter, sys1, sys2), inplist='u', outlist='y')

I don't know how hard these are to fix, it depends on how the underlying interconnection code works. One option would be to just modify the docs to remove mention of auto splitting and summing and add a helper splitter functoin:

def splitter(inputs, outputs):
    D = np.ones((len(outputs),1))
    C = np.zeros_like(D)
    return io.ss2io(ct.ss(0, 0, C, D), inputs=inputs, outputs=outputs)

[1]

python-control/control/iosys.py

Line 1850 in 73f65df

def interconnect(syslist, connections=None, inplist=[], outlist=[],

@murrayrm murrayrm self-assigned this Feb 4, 2021
@murrayrm
Copy link
Member

murrayrm commented Feb 4, 2021 •
edited
Loading

The lack of autosumming is a bug and I can fix that one pretty quickly. Adding a splitter function might also be useful, so I'll probably do that at the same time.

For inplist/inputs and outlist/outputs: as @sawyerbfuller points out, there are two things that a user might want to do:

  • Choose which signals are inputs and outputs (from the set of signals inside the system, including possible sums of those signals)
  • Name the signals

The idea is that inplist/outlist specifies the signals (including allowing summation and splitting) and inputs/outputs names them. Note that if you aren't using the autoconnect functionality then signal names are not unique without the system name prepended (so there might be multiple 'u[0]' signals, but only one 'sys[0].u[0]' signal) => not sure we want to just use the underlying signal name (it could get ugly if you have a couple of levels of nesting, so that your overall system output becomes 'sys[0].sys[1].y[0]' for example). It could also get ugly with implicit summing (an output could become 'sys[0].y[0]+sys[1].y[1]' for example).

@sawyerbfuller
Copy link
Contributor Author

sawyerbfuller commented Feb 4, 2021 •
edited
Loading

The lack of autosumming is a bug and I can fix that one pretty quickly. Adding a splitter function might also be useful, so I'll probably do that at the same time.

🤙

For inplist/inputs and outlist/outputs: as @sawyerbfuller points out, there are two things that a user might want to do:

* Choose which signals are inputs and outputs (from the set of signals inside the system, including possible sums of those signals)

* Name the signals

Makes sense.

The idea is that inplist/outlist specifies the signals (including allowing summation and splitting) and inputs/outputs names them. Note that if you aren't using the autoconnect functionality then signal names are not unique without the system name prepended (so there might be multiple 'u[0]' signals, but only one 'sys[0].u[0]' signal) => not sure we want to just use the underlying signal name (it could get ugly if you have a couple of levels of nesting, so that your overall system output becomes 'sys[0].sys[1].y[0]' for example). It could also get ugly with implicit summing (an output could become 'sys[0].y[0]+sys[1].y[1]' for example).

For the output of an autosumming version of the summing-parallel-systems example above, it makes sense that you would not want the output name be 'y' if it is actually sys1.y + sys2.y (looks like in that case Matlab forces you to create a summing block with a new name).

I think I understand now. I am curious to see how it will work in practice with autosumming.

@murrayrm murrayrm changed the title new iosys.interconnect is great new iosys.interconnect is great, but... Feb 6, 2021
@murrayrm murrayrm mentioned this issue Feb 7, 2021
Merged
@murrayrm murrayrm linked a pull request Feb 7, 2021 that will close this issue
I/O system updates #536
Merged
@sawyerbfuller
Copy link
Contributor Author

I’ve reread the text of this issue, and I think the name change is fair enough : )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@murrayrm murrayrm

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

I/O system updates murrayrm/python-control
2 participants
@murrayrm @sawyerbfuller