ilayn / harold

An open-source systems and controls toolbox for Python3
MIT License
173 stars 19 forks source link

creates a branch with a routh array class #62

Closed MrSamuelLaw closed 2 years ago

MrSamuelLaw commented 4 years ago

adds basic class that uses the Sympy package to allow for the generation of Routh Arrays.

ilayn commented 4 years ago

Hmm, I thought I can handle some sympy but I think I am out of my depth here. I will try to review the Pythonic parts but let's ask others' opinion about this to include in sympy. Because then we can directly create a wrapper here (internally importing from sympy) and we can build upon that basis much better (Kharitonov's theorem etc).

@namannimmo10 @moorepants @eric-wieser

Routh array problems are quite ubiquitous in control education and this looks like a fine addition to the GSOC control program. And Samuel is very kindly implemented this and would like to start contributing. Are you maybe interested in this functionality appearing in sympy instead? I would be happy to help working on this. Please feel free to decline I can imagine it can be tricky to decide if you are not familiar with it.

Also @alchemyst I think this is also a good question for you to chime in since we got some inspiration from your codebase.

alchemyst commented 4 years ago

I've got a slightly more streamlined version of the routh function in the tbcontrol library now, which you are welcome to copy straight over with my blessing. Since tcontrol is also on pypi now, it might also be simpler to just use it when you need this kind of thing?

In terms of this particular code I am not really understanding why this needs to be a class. It feels like quite a bit of the code is re-implementing what you would get for free if you just returned a sympy.Matrix from a function. You get the formatting, the subbing, the indexing etc all right there. If you really want to embed the domain function as a method of some class (instead of just having another function), I think it makes more sense to have this class subclass sympy.Matrix and override the __init__ to take a sympy.Poly as an input and calculate the matrix right there. Then you have all the methods from sympy.Matrix as well as an extra one for domain.

I'd be happy to give a more thorough code review as well if you decide to continue along this route.

moorepants commented 4 years ago

These are the kinds of things we'd like to see in the new sympy control package (https://github.com/sympy/sympy/tree/master/sympy/physics/control). I think a function that consumes a polynomial and outputs the Routh table would be a simple and useful addition. The table could be output as a SymPy matrix with zero's padding the table and the SymPy printing routines for the matrix can handle any printing needs.

I'd have to be convinced of the need for a full blow Routh class. There is also an operation to be had on the data in the Routh table to find if the system is stable or not or the minimal set of symbolic criteria that could guarantee stability. So maybe forming the table, printing the table, formulating the criteria, and deciding if stable or not is complex enough to encapsulate in a class. But maybe a couple of functions is also sufficient.

I recommend submitting PRs to SymPy and you can see what kind of feedback is provided.

MrSamuelLaw commented 4 years ago

Hi everyone, thank you for the feedback. A few things, this is my first time contributing to a project on github, if you have recommendations for what should change in my code or conduct I am all ears. I also realized that some background on why I made the class may be helpful in forwarding the discussion. I am an undergraduate mechanical engineering student and the Routh Array was used heavily during the first portion of the semester in my Intro to Controls class. My professor liked to assign tricky problems with regards to the Routh Array and displaying them as a Sympy Matrix often resulted in messy output that would wrap weirdly in the Jupyter Lab environment. So I decided that creating a Routh Array generator where the ASCII text could be line wrapped would be very helpful. I made it a class instead of just a group of functions because a table is very easy to conceptualize as an object. The closest I wanted to get to determining the stability is to use the domain property once the table was univariable. The reason I decided that was the best options was then the engineer could look and determine which row could be all zeros could solve for that particular case using which ever method they felt most comfortable with. I hope this helps move things forward.

eric-wieser commented 4 years ago

displaying them as a Sympy Matrix often resulted in messy output that would wrap weirdly in the Jupyter Lab environment

The latex output, you mean? Can you paste a screenshot(perhaps in a new sympy issue)? You can also use print(repr(mat)) to force sympy to give plaintext output, or print(sympy.pprint(mat)) to give unicode-art. If both of these are still too ugly, then I'd encourage you to improve the sympy matrix printer itself.

ilayn commented 4 years ago

Hi everyone, thank you for the feedback. A few things, this is my first time contributing to a project on github, if you have recommendations for what should change in my code or conduct I am all ears. I also realized that some background on why I made the class may be helpful in forwarding the discussion.

You are doing just fine. No worries, I wanted to expand the discussion to extended circles because if we get the initial ideas right, then it is often much easier to move forward.

My professor liked to assign tricky problems with regards to the Routh Array and displaying them as a Sympy Matrix often resulted in messy output that would wrap weirdly in the Jupyter Lab environment

Yes, I think you mean when there are two or more parameters things might get a bit complicated inside the cells and expressions get longer. But I think that is a bit inevitable when dealing with symbolic variables. One solution might be to restrict the width of the table and introduce line breaks in the Routh array but I am a bit scared that it would open a can of worms of string parsing and line breaking logic.

The reason I decided that was the best options was then the engineer could look and determine which row could be all zeros could solve for that particular case using which ever method they felt most comfortable with.

I can assure you nobody outside academia uses these. It's purely a teaching tool. So the emphasis can be put on beautifying it or its convenience.

Would you like to open a PR in sympy or would you like to iterate a bit over here and then move the final draft there? All fine with me. I can actually provide some inital functions hopefully tomorrow too

ilayn commented 4 years ago

I've got a slightly more streamlined version of the routh function in the tbcontrol library now, which you are welcome to copy straight over with my blessing. Since tcontrol is also on pypi now, it might also be simpler to just use it when you need this kind of thing?

Much appreciated @alchemyst . We'll definitely check it out.