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

Broadcasting addition for mimo systems #416

Closed
albheim opened this issue Dec 21, 2020 · 4 comments
Closed

Broadcasting addition for mimo systems #416

albheim opened this issue Dec 21, 2020 · 4 comments
Labels
v1 Issues to resolve before releasing v1

Comments

@albheim
Copy link
Member

albheim commented Dec 21, 2020

As mentioned by @olof3 in #166 we currently have that addition works between mimo system and scalars. Addition of scalar matrices also works, but broadcasting does not work.

ss(-ones(2,2), ones(2,2), ones(2,2), zeros(2,2)) + 1 # A, Works
ss(-ones(2,2), ones(2,2), ones(2,2), zeros(2,2)) + ones(2, 2) # B, Works
ss(-ones(2,2), ones(2,2), ones(2,2), zeros(2,2)) .+ 1 # C, Does not work

This operation has to mean that one wants to add something to the D matrix (or equivalently to the matrix of transfer functions) which means we are doing matrix-scalar addition in the mimo case, and it just seems a bit off that we handle it differently then the rest of the julia ecosystem.

I see the problem that we represent a siso system as a 1x1 array of systems and then that would also fall into the same category of matrix-scalar addition, so I'm not sure what would be the nicest solution.

Works fine as it is now so I'm in no hurry to do anything about it, but thought I would document it since I closed the old issue.

@olof3
Copy link
Contributor

olof3 commented Dec 22, 2020

Good that you opened a new issue.

I don't quite remember all of the discussions around this. But here are some thoughts of today.
A seems suspicious, e.g., ones(2,2) + 2 doesn't work, so would probably be best to disallow this
B is fine, this should be allowed
C not sure about this one, it seems like this is not something that one would typically want to do with an LTISystem, and + ones(size(D)) is not overly cumbersome. So for addition of scalars I don't think this is too important.

I could envision that it occasionally would be useful to pre/post-multiply all the output/input channels of a MIMO system with the same SISO system. Not sure if there currently is an easy way of doing that?

@albheim
Copy link
Member Author

albheim commented Dec 22, 2020

I mostly added C since that felt more in line with Julia syntax than A so if we should have either I'd vote for that. But I'd be as fine with just keeping B also since as you say it is not a lot of work to create that matrix.

Pre/post multiplication with siso might be useful some times, I'm not aware of any really easy and clean way to do it.

@mfalt
Copy link
Member

mfalt commented Dec 24, 2020

The following is a rant that I didn't have time to finish thinking through. I think there is something to consider here, but I have not been able to wrap my head fully around it yet. Posting the thought here anyway.

I am a bit surprised that A. works, I don't think it should. I do however think that C should work (?). Same way for multiplication (?), however, it is not trivial to make it work for all cases in a type stable and understandable manner, even with a SISO/MIMO parameter in the type.

For example the case
sys1 .+ sys2.
If sys2 is SISO, then we want to broadcast it as a scalar, and resulting system is ny1 x nu1,
If it is SIMO (ny2 x 1) then we want to make sure that ny1=ny2, and broadcast addition as a column vector to a ny1 x nu1 system.
If it is MISO (1 x nu2) then check nu2=nu1 and broadcast as a row vector to ny1 x nu1
And in the same way when sys1 changes dimensions. But in both cased we have matrices as the underlying types, so we can not rely on multiple dispatch.

With multiplication it gets confusing to me, maybe the only reasonable interpretation is to do it as the case for broadcast over the SISO systems in the TF matrices, but maybe there is another interpretation?
If you think of the systems as block diagrams, sys1 .* sys2 where sys1 is ny1 x nu1, sys2 1x1 could easily be thought to mean that the output of sys2 is used as input to each of the inputs to sys1, i.e sys2*[sys1;sys1;...] but this is a ny1 x 1 system (or if sys2 has multiple inputs a ny1 x nu2 system). Very different than the ny1 x nu1 system we get from seeing it as matrices . But this looks really weird when considering something like [1/s 1/(s^2)] .* 1, here you would expect to get [1/s 1/(s^2)] back.
However there is currently no way (?) to get this "splitting" behavior, which feels reasonable when thinking of the systems in a block diagram.

@olof3
Copy link
Contributor

olof3 commented Dec 24, 2020

I am a bit surprised that A. works, I don't think it should.

Good, let's try to get rid of that. I started a new issue #417 on non-broadcasted addition, where we can discuss that and some related things.

While it might seem natural to support broadcasted addition and multiplication in analogy with matrices, I don't see too many uses of either. By not introducing any of these, I think we will avoid a lot of confusion and headache while not really lose anything in terms of functionality.

To me, the sensible interpretation of "element-wise"/channel-wise multiplication is that every input channel or output channel of the MIMO system is multiplied with the same SISO system. If there would be a reasonably convenient way to form such diagonal systems for pre/post multiplication I think we are better off without introducing .* . I think that would make it more clear what is going on and avoid potential confusion.

.+ seems even less useful, if we figure out how to form those diagonal systems it would be possible to do
sys1 + ones(size(sys1)) * diag(sys2) if someone for some reason wanted to do this. If it's just about adding a constant then it will be even easier.

@baggepinnen baggepinnen added the v1 Issues to resolve before releasing v1 label Oct 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v1 Issues to resolve before releasing v1
Projects
None yet
Development

No branches or pull requests

4 participants