[GSoC] Add TransferFunctionMatrix class in physics.control #19761

namannimmo10 · 2020-07-14T00:02:56Z

References to other Issues or PRs

#19352

Brief description of what is fixed or changed

This PR introduces MIMO transfer functions in sympy.physics.control.

Other comments

Release Notes

physics.control
- Added TransferFunctionMatrix class in physics.control

sympy-bot · 2020-07-14T00:03:06Z

✅

Hi, I am the SymPy bot (v161). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

physics.control
- Added TransferFunctionMatrix class in physics.control (#19761 by @namannimmo10)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.9.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234" (see
https://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->
https://github.com/sympy/sympy/issues/19352

#### Brief description of what is fixed or changed
This PR introduces MIMO transfer functions in `sympy.physics.control`.

#### Other comments


#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
*  physics.control
    * Added `TransferFunctionMatrix` class in physics.control
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

namannimmo10 · 2020-07-14T19:22:26Z

ping @ishanaj!

namannimmo10 · 2020-07-16T15:18:36Z

Ready for review!

codecov · 2020-07-16T19:39:43Z

Codecov Report

Merging #19761 into master will increase coverage by 0.009%.
The diff coverage is 81.366%.

@@              Coverage Diff              @@
##            master    #19761       +/-   ##
=============================================
+ Coverage   75.868%   75.877%   +0.009%     
=============================================
  Files          669       669               
  Lines       173564    173811      +247     
  Branches     40969     41074      +105     
=============================================
+ Hits        131680    131884      +204     
- Misses       36147     36171       +24     
- Partials      5737      5756       +19

namannimmo10 · 2020-07-16T20:21:10Z

I have added a MIMO transfer function object and its important properties. What needs to be done for the interconnection of TransferFunctionMatrix objects? We could perform in-place operations by just implementing __add__ & __mul__. But I guess mimo block diagram will not work after that. Can I start off with two new classes just like Series and Parallel in SISO TF?

Sc0rpi0n101

If we've supported block diagram for SISO systems, I think we should also do the same for MIMO systems.

sympy/core/tests/test_args.py

sympy/physics/control/tests/test_lti.py

namannimmo10 · 2020-07-17T15:46:15Z

OK, I will start working on these classes now!

namannimmo10 · 2020-07-17T19:05:35Z

@moorepants, @sylee957 Can either one of you (or both) also confirm if the work done till now looks fine? Everything else is built on top of TransferFunctionMatrix.

ilayn · 2020-07-17T21:53:51Z

I am just passing by but I think the class name should be TransferMatrix without the Function word.

namannimmo10 · 2020-07-18T10:25:54Z

Hi @ilayn! Thanks for this suggestion. I just got to know about the control systems toolbox you implemented over the years - harold. My first PR for adding SISO transfer function is merged, here's the documentation. The class was named as TransferFunction, so if we change TransferFunctionMatrix to TransferMatrix, then maybe we should also rename TransferFunction to Transfer (the name used in harold for both SISO and MIMO TF).

ilayn · 2020-07-18T12:13:47Z

It is wonderful that you are working on this and it is such a valuable addition. I am indeed using/developing harold but I might add a disclaimer that my wording in harold is highly unconventional so please take it with some salt. It is probably better that others chime in too.

My logic is that Transfer function name is like "he is such a gentleman man" that is Transfer function is already a function no need to emphasize it. Same with state space that actually means basically R^n so again redundant wording. Hence the naming Transfer and State models. But again this is not a very popular view so you have been warned :-)

Another option is to have a single object TransferFunction and let it handle MIMO systems too to reduce clutter.

sylee957 · 2020-07-18T15:04:34Z

I think that it's different matter for naming between TransferFunction vs Transfer and TransferFunctionMatrix vs TransferMatrix because both terms Transfer Function Matrix and Transfer Matrix are used academically
https://en.wikipedia.org/wiki/Transfer_function_matrix

When I search how many IEEE papers out there for Transfer Function Matrix and Transfer Matrix
https://ieeexplore.ieee.org/search/searchresult.jsp?newsearch=true&queryText=%22Transfer%20Function%20Matrix%22
https://ieeexplore.ieee.org/search/searchresult.jsp?newsearch=true&queryText=%22Transfer%20Matrix%22
I see there Transfer Matrix is searched more

But I don't think that Transfer Matrix is better to use because of the plain reason that there are more papers using them, because the term Transfer Matrix can confuse with Transfer Matrix Method while Transfer Function Matrix is more accurate that it is used only by a handful of control theory journals.

namannimmo10 · 2020-07-18T21:15:14Z

What should we name these two classes for series and parallel interconnection? What about TFMSeriesConnect and TFMParallelConnect?

sympy/physics/control/lti.py

ilayn · 2020-07-21T16:41:01Z

When I search how many IEEE papers out there for Transfer Function Matrix and Transfer Matrix
https://ieeexplore.ieee.org/search/searchresult.jsp?newsearch=true&queryText=%22Transfer%20Function%20Matrix%22
https://ieeexplore.ieee.org/search/searchresult.jsp?newsearch=true&queryText=%22Transfer%20Matrix%22
I see there Transfer Matrix is searched more

I don't think IEEE is doing a good job here by returning a couple of thousand results. This terminology should in either case return way more results. And the terminology clash is sufficiently far apart, for example, I was completely immersed in control theory and not once I heard this method. But anyways, my vote goes out for having a single class with SISO/MIMO as special cases.

eric-wieser · 2020-08-28T14:36:38Z

@oscarbenjamin

The tfm3 case doesn't seem right to me. I don't think it should be possible to have a Matrix of non-Expr transfer function objects.

Why can't TransferFunction be an Expr? If the issue is Add and Mul, then it seems that Series is like MatMul and Parallel is like MatAdd. Out of scope for this patch, but if we did that we could avoid reimplementing matrix multiplication in this PR.

eric-wieser · 2020-08-28T14:38:23Z

sympy/physics/control/lti.py

+                raise TypeError("Var must be a Symbol, not {}.".format(type(var)))
+
+        obj = super(TransferFunctionMatrix, cls).__new__(cls, arg)
+        if all(isinstance(i, (TransferFunction, Series, Parallel)) for i in arg):


This path needs to check that Series and Parallel are all is_SISO, right?

sympy/physics/control/lti.py

eric-wieser

The special-casing of num_input and num_outputs being 1 scares me, and I think it will make this fail:

a1 = TransferFunctionMatrix([-tf4], (1, 1), p)
a11 = TransferFunctionMatrix([[-tf4]], (1, 1), p)
b2 = TransferFunctionMatrix([-tf4, tf4], (2, 1), p)
b21 = TransferFunctionMatrix([[-tf4, tf4]], (2, 1), p)
c = TransferFunctionMatrix([[-tf4], [tf4]], (1, 3), p)

# all of these products should be legal
for a in [a1, a11]:
    for b in [b1, b11]:
        b * a
    a * c

Sc0rpi0n101 · 2020-08-28T15:07:49Z

before the GSoC period ends.

When is that?

August 31

sympy/physics/control/tests/test_lti.py

Sc0rpi0n101 · 2020-08-29T11:56:56Z

@namannimmo10 can you push the work you have done on the documentation?

Also, please respond to @eric-wieser's comments. We have less than 2 days till the deadline.

namannimmo10 · 2020-08-30T00:43:00Z

Some of the comments are still to be addressed, and class level docstring will be added soon. (Didn't add in this commit because some API design changes are to be made).

eric-wieser · 2020-08-31T09:45:44Z

sympy/physics/control/lti.py

+            if not isinstance(var, Symbol):
+                raise TypeError("Var must be a Symbol, not {}.".format(type(var)))
+
+        obj = super(TransferFunctionMatrix, cls).__new__(cls, arg)


I'm starting to think you really do want to reuse the matrix classes here. So I'd propose __new__ looks like:

def __new__(cls, arg, shape, var): if var is not None and not isinstance(var, Symbol): raise TypeError("Var must be a Symbol, not {}.".format(type(var))) if shape is not None and not isinstance(shape, tuple): raise TypeError("Shape must be a tuple, not {}.".format(type(shape))) # let matrix handle conversion from lists matrix = ImmutableDenseMatrix(arg) # args[2] is the flattened entries for entry in matrix.args[2]: if not isinstance(entry, (TransferFunction, Parallel, Series)): raise TypeError("Unsupported type for argument of TransferFunctionMatrix.") if var is not None and entry.var != var: raise ValueError("All transfer functions should use the same complex" " variable (var) of the Laplace transform.") if shape is not None and matrix.shape != shape: raise ValueError("wrong shape") # or a better message obj = super(TransferFunctionMatrix, cls).__new__(cls, matrix) obj._var = var # etc return obj

A big advantage of this approach is that TransferFunctionMatrix.subs(some_variable, 0) will just work.

Does subs not work with this class?

There is a basic problem in sympy with allowing non-Expr (even non-Basic) instances in places such as Matrix entries that are clearly written for Expr instances. I intend to eliminate all places where non-Expr instances are used in a Matrix from the codebase and deprecate having non-Expr in a Matrix.

I don't think that it is a big deal to implement something like matrix multiplication which is fairly trivial although perhaps it would be better if there was a reusable function somewhere for doing that calculation. It is still a work in progress and not at all documented but I have implemented various matrix routines in such a way that they can be applied to a standard Python list of lists based on only assuming that the elements support the field operations +, -, * and / so do not need to Expr or even Basic. My intention was that it should not be necessary to subclass Matrix and insert objects that don't belong there just to reuse the routines for standard matrix operations. Matrix multiplication is handled by this function:

sympy/sympy/polys/domainmatrix.py

Lines 220 to 226 in 500f3c9

def ddm_imatmul(a, b, c):

"""a += b @ c"""

cT = list(zip(*c))

for bi, ai in zip(b, a):

for j, cTj in enumerate(cT):

ai[j] = sum(map(mul, bi, cTj), ai[j])

That implementation is optimised for large matrices and cheap elementary arithmetic (e.g. int or mpz as the elements).

Does subs not work with this class?

My impression was that subs would not recurse into list arguments, but perhaps I missed a sympify somewhere.

At any rate, my suggestion above is more about letting the matrix constructor handle the normalization, and less about the multiplication itself. The thing I don't like about this PR is how the special cases for TFM([tf]) vs TFM([[tf]]) are in every single method, when they should just be in the constructor.

I intend to eliminate all places where non-Expr instances are used in a Matrix from the codebase and deprecate having non-Expr in a Matrix.

If you do this, I'd argue that it might be worth introducing a base class that works over non-exprs too. In my mind, this control stuff is really trying to work with a new TransferFunctionExpr type, just like matrices work with a Matexpr type.

I'd argue that it might be worth introducing a base class

The Matrix class is already arranged into a bunch of different base classes:

In [1]: Matrix.mro() Out[1]: [sympy.matrices.dense.MutableDenseMatrix, sympy.matrices.dense.DenseMatrix, sympy.matrices.matrices.MatrixBase, sympy.matrices.matrices.MatrixDeprecated, sympy.matrices.matrices.MatrixCalculus, sympy.matrices.matrices.MatrixEigen, sympy.matrices.matrices.MatrixSubspaces, sympy.matrices.matrices.MatrixReductions, sympy.matrices.matrices.MatrixDeterminant, sympy.matrices.common.MatrixCommon, sympy.matrices.common.MatrixArithmetic, sympy.matrices.common.MatrixOperations, sympy.matrices.common.MatrixProperties, sympy.matrices.common.MatrixSpecial, sympy.matrices.common.MatrixShaping, sympy.matrices.common.MatrixRequired, sympy.printing.defaults.Printable, object]

It's just not really clear how you could make use of any of those classes separately.

Sc0rpi0n101 · 2020-10-04T02:16:32Z

Hey @namannimmo10,

@oscarbenjamin is planning to release SymPy 1.7 soon. If you are free and available now, can you finish this PR so that this work can also be included in the release?

namannimmo10 · 2020-10-04T17:16:34Z

Thanks! Yes, I am planning to complete this PR soon.

Sc0rpi0n101 · 2020-10-12T01:32:08Z

Any progress on this @namannimmo10?

oscarbenjamin · 2020-11-19T22:19:23Z

After a significant delay I'm getting ready for the 1.7 release. Is anything going to happen in this PR soon?

oscarbenjamin · 2020-11-19T22:23:29Z

I'm removing the 1.7 milestone. Let me know if there any changes come soon that should be considered for 1.7

czgdp1807 · 2020-12-08T07:26:39Z

@namannimmo10 Can this become a part of next GSoC?

white-peed · 2021-03-11T06:38:01Z

I would like to work on this issue as my GSoC project. I know basic Git/Github, Python and Control Systems(via NPTEL lectures). Currently I am reading the documentation and code of the the matrix. Please help me get started.

namannimmo10 · 2021-03-11T13:57:07Z

Sure, please feel free to take over.
Some comments need to be addressed first; let us know what you think about the existing implementation and how it can be made more robust.

white-peed · 2021-03-18T12:36:01Z

Sure, please feel free to take over.
Some comments need to be addressed first; let us know what you think about the existing implementation and how it can be made more robust.

I was going over the document http://www.cds.caltech.edu/~murray/amwiki/index.php/Second_Edition , and I am currently having the following ideas:-

MIMO systems :- I think using a 2D array would work well in this case.
State space systems :- I am currently studying this topic.
Documentation :- Few more examples of first order systems and second order can be added.
pole zero function :- This can probably be done by using the roots function, for plotting I am thinking of using the matplotlib.
root locus :- I was thinking of using a large number of data points with the plot function to plot the data. Numpy preprocessed functions could not be used as we don't which function to apply.

namannimmo10 · 2021-03-21T09:19:53Z

@white-peed, I think some small steps can be taken to further develop sympy.physics.control. That way we can discuss in detail each step!

One task is to complete this outstanding PR; it includes resolving the merge conflicts, addressing some comments, and making TransferFunctionMatrix more robust (after some discussion which is pending by one of the reviewers).
Another important task is to add 3-4 working examples in the documentation showing the users how to exercise this package.
After that, we can think of adding graphical analyses to it. pole_zero and root_locus will simply use .poles() and .zeros() methods in TransferFunction class. And SymPy's own plotting module will work fine. bode plot should also be added.
Finally, once all the aforementioned tasks are handled, the StateSpace class can be introduced for creating State Space models. I would also refer to python-control and harold for doing this task.

All in all, I believe some examples are essential as the development proceeds.

white-peed · 2021-03-26T03:06:18Z

Can you explain " making TransferFunctionMatrix more robust " ?

…

On Sun, Mar 21, 2021 at 2:50 PM Naman Gera ***@***.***> wrote: @white-peed <https://github.com/white-peed>, I think some small steps can be taken to further develop sympy.physics.control. That way we can discuss in detail each step! 1. One task is to complete this outstanding PR; it includes resolving the merge conflicts, addressing some comments, and making TransferFunctionMatrix more robust (after some discussion which is pending by one of the reviewers). 2. Another important task is to add 3-4 working examples in the documentation showing the users how to exercise this package. 3. After that, we can think of adding graphical analyses to it. pole_zero and root_locus will simply use .poles() and .zeros() methods in TransferFunction class. And SymPy's own plotting module will work fine. bode plot should also be added. 4. Finally, once all the aforementioned tasks are handled, the StateSpace class can be introduced for creating State Space models. I would also refer to python-control <https://github.com/python-control/python-control> and harold <https://github.com/ilayn/harold> for doing this task. All in all, I believe some examples are essential as the development proceeds. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19761 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APEYJ2UCGD4JWNMFF47Q4P3TEW24RANCNFSM4OY7KYYQ> .

namannimmo10 · 2021-03-28T16:14:18Z

Can you explain " making TransferFunctionMatrix more robust " ?

Please have a look at all the discussions in this PR. There were some suggestions given to improve the existing implementation.

bugmaker2 · 2023-03-27T13:19:35Z

Hi @namannimmo10 . I am also interested in taking Symbolic Control System as my GSoC project. I am still reviewing the knowledge of Control Theory. It takes a few more days before I can comment on the implementation.

Also hello @white-peed . I feel so unlucky as I come here late and you have already come up with some brilliant ideas. Hope there is a chance that we both can contribute to this project.

As I am browsing the comment history, I found @white-peed had mentioned using matplotlib and numpy

pole zero function :- This can probably be done by using the roots function, for plotting I am thinking of using the matplotlib.

Then I found that @namannimmo10 was using matplotlib and numpy in control_plot as well. emm...

I think maybe we should take a look at the Why SymPy section of Introduction to SymPy. It says

An advantage of SymPy is that it is lightweight. In addition to being relatively small, it has no dependencies other than Python, so it can be used almost anywhere easily.

We should follow the philosophy of the design. Also the function would become buggy if the dependencies update in the future. The Sympy has already implemented plotting methods in plotting.pygletplot, we may utilize that.

The idea above is just my random thoughts. I haven't come up with other ideas. I have to read the reference book quickly. See you days later.

initial commit

bea4dea

namannimmo10 marked this pull request as draft July 14, 2020 00:03

namannimmo10 added GSoC physics.control labels Jul 14, 2020

namannimmo10 mentioned this pull request Jul 14, 2020

[GSoC] Tracking tasks for adding control package #19352

Open

4 tasks

add unit tests for printing and implementation for some methods

fee062f

namannimmo10 marked this pull request as ready for review July 14, 2020 19:17

remove unused import

f0d6824

namannimmo10 requested review from moorepants, Sc0rpi0n101 and sylee957 July 14, 2020 19:21

complete implementation and add unit tests

52031aa

make code more pythonic, add test for args

67f0342

Sc0rpi0n101 reviewed Jul 17, 2020

View reviewed changes

sympy/core/tests/test_args.py Show resolved Hide resolved

sympy/physics/control/tests/test_lti.py Outdated Show resolved Hide resolved

namannimmo10 mentioned this pull request Jul 17, 2020

SymPy Control Package python-control/python-control#412

Closed

make changes in Parallel class to support MIMO TF

862dfef

Sc0rpi0n101 reviewed Jul 21, 2020

View reviewed changes

sympy/physics/control/lti.py Outdated Show resolved Hide resolved

sympy/physics/control/lti.py Show resolved Hide resolved

eric-wieser reviewed Aug 28, 2020

View reviewed changes

sympy/physics/control/lti.py Outdated Show resolved Hide resolved

eric-wieser requested changes Aug 28, 2020

View reviewed changes

eric-wieser reviewed Aug 28, 2020

View reviewed changes

sympy/physics/control/tests/test_lti.py Show resolved Hide resolved

Apply suggestions and add docs

19a4c3c

Add class level docstring

4eb606e

eric-wieser reviewed Aug 31, 2020

View reviewed changes

namannimmo10 added this to the SymPy 1.7 milestone Oct 2, 2020

oscarbenjamin removed this from the SymPy 1.7 milestone Nov 19, 2020

czgdp1807 added the Please take over label Dec 8, 2020

This was referenced May 28, 2021

[GSoC] Improving sympy.physics.control : Phase 1 #21539

Closed

[WIP][GSoC] Add TransferFunctionMatrix class in physics.control #21556

Closed

namannimmo10 merged commit c35ae69 into sympy:master Jul 2, 2021

	def ddm_imatmul(a, b, c):
	"""a += b @ c"""
	cT = list(zip(*c))

	for bi, ai in zip(b, a):
	for j, cTj in enumerate(cT):
	ai[j] = sum(map(mul, bi, cTj), ai[j])

Uh oh!

[GSoC] Add TransferFunctionMatrix class in physics.control #19761

[GSoC] Add TransferFunctionMatrix class in physics.control #19761

Uh oh!

Conversation

namannimmo10 commented Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

Release Notes

Uh oh!

sympy-bot commented Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

namannimmo10 commented Jul 14, 2020

Uh oh!

namannimmo10 commented Jul 16, 2020

Uh oh!

codecov bot commented Jul 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

namannimmo10 commented Jul 16, 2020

Uh oh!

Sc0rpi0n101 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

namannimmo10 commented Jul 17, 2020

Uh oh!

namannimmo10 commented Jul 17, 2020

Uh oh!

ilayn commented Jul 17, 2020

Uh oh!

namannimmo10 commented Jul 18, 2020

Uh oh!

ilayn commented Jul 18, 2020

Uh oh!

sylee957 commented Jul 18, 2020

Uh oh!

namannimmo10 commented Jul 18, 2020

Uh oh!

Uh oh!

Uh oh!

ilayn commented Jul 21, 2020

Uh oh!

eric-wieser commented Aug 28, 2020

Uh oh!

eric-wieser Aug 28, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eric-wieser left a comment

Choose a reason for hiding this comment

Uh oh!

Sc0rpi0n101 commented Aug 28, 2020

Uh oh!

Uh oh!

Sc0rpi0n101 commented Aug 29, 2020

Uh oh!

namannimmo10 commented Aug 30, 2020

Uh oh!

eric-wieser Aug 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oscarbenjamin Aug 31, 2020

Choose a reason for hiding this comment

Uh oh!

eric-wieser Aug 31, 2020

Choose a reason for hiding this comment

Uh oh!

eric-wieser Aug 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oscarbenjamin Aug 31, 2020

Choose a reason for hiding this comment

namannimmo10 commented Jul 14, 2020 •

edited

Loading

sympy-bot commented Jul 14, 2020 •

edited

Loading

codecov bot commented Jul 16, 2020 •

edited

Loading

eric-wieser Aug 31, 2020 •

edited

Loading

eric-wieser Aug 31, 2020 •

edited

Loading