Discussion regarding the API for control package

[namannimmo10]

Hello everyone,

Before this, some contributors made efforts to add control package to sympy.physics here and here.

There is a great python library for the operations of control systems.
https://python-control.readthedocs.io/en/latest/index.html
But I want sympy to have a symbolic version of that.

A basic overview of what needs to be added can be found here. But I think some more operations can also be added to enhance the toolbox. Like given in this link: https://www.wolframalpha.com/examples/science-and-technology/engineering/control-systems/

I have already raised a Pull request for adding such toolbox and this can be done in these 5 steps:

• Discussion regarding the API and what operations can be added.
• Addressing all the comments mentioned in Added control package to sympy.physics #17866 and Controlsys package #9916
• Discussing class design and prototypes.
• Final implementation along with unit tests.
• Adding Documentation for all the classes/functions.

Any advice from the community would be really appreciated.

[cekbote]

I totally agree. In addition to that, we could have a combined control system, as well as a signal processing package as both applications, go hand in hand (to a certain extent).

I had raised a PR to try and include magnitude response, phase responses, magnitude plots as well as phase plots, finding out the poles and zeros of an equation as well as pole-zero plots. The link to the PR can be found here [PR #18419.]. I think this could be added to the control package.

I also wanted to add some other features that could help in the signal processing domain as well as the control system domain. I have elaborated my ideas in this discussion: https://groups.google.com/forum/#!msg/sympy/K5i87mX8Dwo/hCxjVzk6EwAJ.

[namannimmo10]

I agree that these disciplines overlap to a certain extent. But it's just that they are separated by a set of common tools.

[cekbote]

ping @oscarbenjamin @moorepants @jksuom

[sylee957]

In my opinion, TransferFunctionModel and StateSpaceModel can subclass Basic and can be sympifyable.
I can say that they are quite well-defined symbolic models used in conventional softwares like Mathematica. Of course, I don't say that every objects should be sympifyable, but things can be complicated later if it does not follow the sympy standard design patterns and later if you are trying to add something like symbolic representations of interconnecting the modules.

Often, system model can be a candidate for a full symbolic model more than just a simple solver.

[namannimmo10]

Wouldn't it be better if we write this module as a set of few functions?

In my opinion, TransferFunctionModel and StateSpaceModel can subclass Basic

Maybe that is better if we're trying to use other sympy methods such as subs etc.. Is that the reason why you want those two classes to subclass Basic?

I don't say that every objects should be sympifyable, but things can be complicated later if it does not follow the sympy standard design patterns and later if you are trying to add something like symbolic representations of interconnecting the modules.

OK, I'll consider that when I'll implement those 2 classes.

[sylee957]

It can be different if it is rewritten as a set of functions, than writing to Basic would not be an issue for a while

But if anything new is going to be implemented as a class and that wraps around sympy types, then it's better to be inheriting Basic from the first place. For example, there were obvious needs of subs, _repr_latex_ and they had to be redefined in any way for these models.

[namannimmo10]

Alright, TransferFunctionModel and StateSpaceModel will subclass Basic.

Regarding the API for StateSpaceModel,
I think it would be better if we do this... Like the way it is implemented here. What do you think?

[sylee957]

What kind of features do you think that StateSpaceModel is lacking for python-control?

I think that most of the stuff already mirrors python-control.

[namannimmo10]

This would be a symbolic version of linear control systems. I guess that would be one of the reasons..
Most of the things mentioned in Appendix A of this thesis (which is in the references) are there in python-control..

[sylee957]

StateSpaceModel already uses sympy matrix, so it would still be able to support most of the symbolics.

However, there can be some differences of enabling symbolics partially in computation, or making the whole model symbolic.
Do you mean that it should be implemented as the latter way?

[namannimmo10]

or making the whole model symbolic.

I don't know how difficult that would turn out to be. Please refer this comment.
But if that's what is required to differentiate it from python-control, then I'm willing to move forward.....
But for that, I need some guidance.

[sylee957]

Often, Basic offers safe immutable design patterns and can resolve lots of issues regarding rewriting, printing, pickling or such that and I think that this is one of the reasons that most of the stuff should be implemented as Basic possible.

I think that converting the objects to the form that are compatible with Basic is simple.

1. I'd first try to define the constructor with __new__ than __init__, such that it sympifies the arguments and

   • StateSpaceModel uses Basic.__new__(cls, A, B, C, D) signature where A, B, C, D are matrices.
   • TransferFunctionModel uses Basic.__new__(cls, num, denom) signature where num, denom are polynomials. each representing the numerators and denominators of the transfer function

   represent, block_represent should be derived from the .args, rather than storing the represent, block_represent by mutating the objects.

2. I'd try to move __str__, __repr__, _repr_latex_ to _print_TransferFunctionModel and _print_StateSpaceModel in StrPrinter, LatexPrinter

3. __eq__ don't have to be defined if it inherits Basic.

4. I'd write _eval_rewrite_as_TransferFunctionModel and _eval_rewrite_as_StateSpaceModel. And then the model conversion can be possible by .rewrite(StateSpaceModel)

5. I'd not define __getattr__ to inherit the methods from the wrapped objects, because Matrix can have a lot of obscure stuff. But I'd design some stuff like jordan decomposition carefully.

And I'd believe that other things would be easier.

[namannimmo10]

And then the model conversion can be possible by .rewrite(StateSpaceModel)

That is a great idea :)

Seems like there are so many benefits while we subclass Basic. I'll include all this in my proposal, and I'll also try to add some other methods which are there in python-control but not in #17866
For instance -- isObservable, RootLocusPlot, Nyquist and Bode plots and some more that can be useful for engineering point of view.

Thanks for the suggestions, @sylee957

[sylee957]

Yes, but I'd try to name is_observable, root_locus_plot, nyquist_plot, bode_plot for naming consistency.

[namannimmo10]

OK. I'll follow snake_case naming style.

[sylee957]

I'd also make anchor an additional symbol s in the arguments like Basic.__new__(cls, A, B, C, D, s) and Basic.__new__(cls, num, denom, s) if it can allow symbolic coefficients. (For numeric cases, s may not be needed like done in python-control, but for allowing symbolic coefficients, there should be a way to anchor the symbol s like done in mathematica.)

[moorepants]

Maybe s should be optional, i.e. s=Symbol('s'). The user will need to be able to access what s is in the transfer functions for certain downstream manipulations.

[sylee957]

But is it a good idea to specify s as Symbol than Dummy, considering the cases where s is not meant to be anything but coefficients.
I'm even thinking of internally using a model with anonymous symbols.

[moorepants]

I still don't understand what a Dummy is. So I don't know. But this is similar to the Beam module where you formulate polynomials in the class methods and need access to the independent variable for further analysis. @oscarbenjamin gave some nice suggestions on how to handle this in an issue somewhere. I forget where...

[moorepants]

Here: #17714 is where he proposed ideas for dealing with the implicit variable for time in the physics vector package.

[moorepants]

I just read https://docs.sympy.org/1.5.1/modules/core.html?highlight=dummy#sympy.core.symbol.Dummy and see that it is a symbol that ensures uniqueness regardless of its string base. So my question is then what advantage does this give over a symbol in this case? Why is it important to make the independent variable of the polynomial unique?

[sylee957]

It's because I'm thinking of defining the equalities between each system models not to depend on the names of the variables.
But of course, it doesn't have to be mandatory.

[moorepants]

I see. That could be valuable.

[namannimmo10]

Apart from the methods that are already there in #17866, these are the following methods that I propose to add in the StateSpaceModel class. I'll address all the comments mentioned in #17866, #9916 and this issue and get the already implemented methods polished for merging.

def observability_matrix(self):
      # Description can be found here: 
      # https://in.mathworks.com/help/control/ref/obsv.html#f3-209625
      # The matrix would be of np rows and n columns. 

def observable_subspace(self):
      # The observable subspace only depends on A and C.
      # And the observable subspace of lti system is equal to the image of its observability matrix.
      
      return self.observability_matrix().columnspace()

def is_observable(self):
      # Returns a Boolean whether or not the system would be observable.
      # If the observability matrix is non-singular, then the system would be observable.
      # According to theory, we can just find the determinant. 
      # If the det is 0, then the system is not observable else it is observable..
      # Or we can do by using `for` loops.

def __neg__(self):
      # Negates a state space system.
      
      return StateSpaceModel(self.args[0], self.args[1],  -self.args[2], -self.args[3])

def _eval_rewrite_as_TransferFunctionModel(self):
      # After this, model interconversion would simply be `.rewrite(TransferFunctionModel)`.

def series(self, other):
      # Returns the series interconnection of the system and another system (other).
      # The implementation in #17866 can be improved further
      # Like using `.args` instead of `.represent` and much more.     

I'll also update with the methods for TransferFunctionModel class. Please let me know if I've missed something or if I'm wrong somewhere.

[sylee957]

There are also some ideas from #12189 to grab.

[namannimmo10]

Cool. Noted!

[namannimmo10]

Also, Can anyone please let me know who would be my potential mentor for this project? So that I discuss all the things with him/her in advance. Thanks.

[czgdp1807]

Take a look at #7977 (comment) as well.

[namannimmo10]

Hi everyone,
I just started working on my GSoC application. Please look into this and let me know if changes are required.
As discussed before, the project can be done in 4 phases.

Phase 1: Adding StateSpaceModel and TransferFunctionModel class. These guidelines will be followed so as to make the whole model symbolic. I'll address all the comments in #17866 and #9916.

Phase 2: In this phase, I will implement more methods such as observability_matrix, observable_subspace, is_observable, __neg__ (For StateSpaceModel), __neg__ (for TransferFunctionModel), _eval_rewrite_as_TransferFunctionModel, _eval_rewrite_as_StateSpaceModel.

Phase 3: At the starting of this phase, I'll add tests for the work done in Phase 1 and Phase 2.
Documentation will also be added. Control package would be implemented by then. After that, I will add convenience function to lambdify the StateSpaceModel. Test for the same will also be added.

Phase 4: In this phase, I will add nyquist_plot and root_locus_plot (for TransferFunction). Finally, I'll add documentation and complete all the things that would still be unmerged by then.

@moorepants @sylee957 Please give some reviews on this. I'll add Phase 1 implementation plan in my application tomorrow. I want to start working on this project as soon as I can so as to maximize my chances of selection.
Looking forward to hearing from you both. :)

[moorepants]

This all sounds cool. I think the Bode plot is used much more widely than the Nyquist plot, so I'd implement that before Nyquist.

[namannimmo10]

Thanks, @moorepants
I'll add the Bode plot instead of the Nyquist plot. Even I was initially confused, but now I also think the bode plot should be implemented first. So the bode_plot and root_locus_plots are fine??
We can use SymPy's own plotting module to implement that.

I think we should have plenty of examples in the docs. Because people tend to use the software more if the docs are top-notch.

[moorepants]

I think a Bode plot and root locus are two of the most popular control related plots used.

[namannimmo10]

Alright, we can add them. One is a frequency response plot and the other is a root locus plot for the transfer function. Should I write the prototypes for all the features that I will add?

[namannimmo10]

Should I write the prototypes for all the features that I will add?

Or just the outside APIs would suffice?

[oscarbenjamin]

I think that you should write tests for each part at the same time as implementing it. Writing tests afterwards is not usually a good idea as you will forget what needs to be tested. Writing tests at the same time helps to ensure that everything is implemented correctly.

[moorepants]

I agree with @oscarbenjamin. And a GSoC proposal that shows examples of use (like the "examples" sections in SymPy documentation) or even unit tests will provide a very clear picture of what you want to do.

[namannimmo10]

Thanks for the suggestions. I'll write tests in the last week of each phase.

[namannimmo10]

I have added Phase 1 implementation plan. Please check this and leave comments if any changes are required. Thanks!

[namannimmo10]

@moorepants We can drop evaluate for numeric solutions to the system (as mentioned in #17866 (comment), and focus only on symbolic evaluation and improving it even more. I'll update this in my application. Is there anything else that needs to be addressed or if I'm wrong somewhere as per my proposal, or should I move forward? Thanks!

[oscarbenjamin]

I think that numerical solutions can be fine if this is something that is added generally in sympy for working with ODEs i.e. #18023

[namannimmo10]

@moorepants, I have completed Phase 1, 2, and almost 4. Please review my proposal whenever you get time. I just want to make sure I'm doing it the right way. Thanks.

[namannimmo10]

@moorepants Please give your final feedback on my application. Looking forward to hearing from you.

[czgdp1807]

Can this, #18436 be closed? Or #18436 is going to be continued?
@moorepants @namannimmo10

[namannimmo10]

I want to work on a new PR. Please address this comment. If you agree, then we can close #18436

[namannimmo10]

I want to work on a new PR

Actually, i want to copy the file from the most recent commit in #17866 and start my work from there in a new PR.