Updates to balred() and gram() #118

mdclemen · 2016-10-21T00:14:06Z

balred() now supports "matchdc" and will handle unstable systems and will accept a list of ORDERS, which will return a list of reduced order systems. gram() now does Cholesky factored gramians if desired. Requires my updates to slycot.

coveralls · 2016-12-25T03:26:51Z

Coverage decreased (-0.2%) to 75.797% when pulling f2bdde4 on mdclemen:master into cdd3e73 on python-control:master.

slivingston · 2016-12-26T20:19:34Z

control/statefbk.py

@@ -347,6 +347,9 @@ def gram(sys,type):
    #Check for ss system object
    if not isinstance(sys,statesp.StateSpace):
        raise ValueError("System must be StateSpace!")
+    gramtypes = ['c', 'o', 'cf', 'of']


Why do you declare gramtypes as a variable? It only appears to be used in the conditional expression on the next line, so the motivation for allocating a variable with function-level scope is not apparent to me.

I don't really remember, I wrote that function a few months ago. If you think there is a better way to do things, then by all means change it.

For the reason that I gave above, moving the list into the conditional expression improves code quality, and thus, I recommend the change. However, it is not critical for accepting this pull request.

Just to be clear, I am belatedly providing review of your proposed changes now.

slivingston · 2016-12-26T20:23:36Z

control/modelsimp.py

@@ -50,7 +50,7 @@
 from .statesp import StateSpace
 from .statefbk import gram

-__all__ = ['hsvd', 'balred', 'modred', 'era', 'markov', 'minreal']
+__all__ = ['hsvd', 'balred', 'modred', 'era', 'markov', 'minreal',]


Why did you add a trailing , in this list expression? It is not a syntax error, but it is not consistent with the current style of the control package.

I forgot to delete the trailing ','.

Can you delete it, then? I am writing additional comments now, so you might want to wait before rebasing until I send them all.

slivingston · 2016-12-28T05:21:52Z

In the first commit (cc4b5c7) of this pull request, the message and author are incorrectly copied from cdd3e73. When I skim the diff, it appears that you applied changes as part of your own work, but then, those were combined with the patch corresponding to commit cdd3e73. Can you clean it up, in particular remove the re-applied pieces of commit cdd3e73?

slivingston · 2016-12-28T05:34:33Z

It appears that substantial amounts of code are added in the first commit and then deleted or modified in the second. Can you add details in the commit messages to explain this transition? Alternatively, can you squash changes into a single commit?

Those are all of my questions and requests for now. I will wait until you respond or rebase before continuing to review.

mdclemen · 2017-01-03T17:46:59Z

I made changes to the code that you recommended, but my git skills might not be good enough for the other things you want me to do.

slivingston · 2017-01-06T18:00:20Z

@mdclemen Can I have your permission to create a commit that has your changes from this PR and lists you as author (and me as committer)? This way I can make the rebasing adjustments that I requested above. If I do this, how do you prefer your name to be written? E.g., I can use "clementm" as in your commit f74e665, or your name as it appears in your publications.

Alternatively, we can arrange to meet in IRC to work through the rebase process together.

… will accept a list of 'orders'. gram() now does Cholesky factored gramians if desired. Requires slycot routines AB09MD, AB09ND, SB03OD.

slivingston · 2017-01-18T22:12:39Z

control/modelsimp.py

@@ -211,26 +218,46 @@ def balred(sys, orders, method='truncate'):
        of systems)
    method: string
        Method of removing states, either ``'truncate'`` or ``'matchdc'``.
+    alpha: float
+        Specifies the alpha-stability boundary for the eigenvalues


This text description of alpha appears to be copied from AB09MD.f of the SLICOT source code. Is that true? SLICOT is under the GNU GPL, whereas we want to keep this control package under the BSD license.

If the text was indeed copied, can you create a new description of the alpha parameter? An easy solution for now is to write a brief summary and indicate where more information can be found (the documentation of SLICOT).

The original text from SLICOT to which I am referring is in lines 89-96 of AB09MD.f

I did in fact copy the description of alpha from AB09MD.f.

slivingston · 2017-01-18T22:30:42Z

control/modelsimp.py

    """
    Balanced reduced order model of sys of a given order.
    States are eliminated based on Hankel singular value.
+    If sys has unstable modes, they are removed, the
+    balanced realization is done on the stable part, then
+    reinserted IAW reference below.


What is "IAW"?

can you write "in accordance with" explicitly? The abbreviation might not be widely recognized.

I can also make the change after merging.

mdclemen · 2017-01-18T22:46:56Z

In Accordance With

…

On Wed, Jan 18, 2017 at 2:30 PM, Scott C. Livingston < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In control/modelsimp.py <#118 (review)> : > """ Balanced reduced order model of sys of a given order. States are eliminated based on Hankel singular value. + If sys has unstable modes, they are removed, the + balanced realization is done on the stable part, then + reinserted IAW reference below. What is "IAW"? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#118 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHIA6356Dhq1s6a8jG4dZMEYUEh2E077ks5rTpKTgaJpZM4KcvGY> .

slivingston · 2017-01-18T22:51:01Z

I resolved the merge conflicts, but before I apply them here, can you first address my question about the description of the alpha parameter?

slivingston · 2017-01-18T23:09:46Z

Should we keep the original call to AB09AD when the system is stable? Otherwise (if method='truncate' and the system is not stable), your newly introduced call to AB09MD is used.

The Fortran implementations of these two subroutines are different, so I am motivated to understand them more before removing the one dedicated to the case of stable systems.

slivingston · 2017-01-18T23:14:17Z

After we decide actions regarding AB09AD and the description of the alpha parameter, then this PR will be ready for merging. Thanks for your good work on the gram and balred functions!

mdclemen · 2017-01-19T18:41:50Z

I don't know why the SLICOT authors decided to include a user defined boundary for what constitutes unstable eigenvalues but essentially alpha allows the user to redefine the boundary for a stable eigenvalue.

…ibe `alpha`. In balred(), if method='truncate' and the system is stable, slycot ab09ab is used, otherwise ab09md is used.

mdclemen · 2017-01-19T19:21:54Z

@slivingston let me know what you think of my updates.

slivingston · 2017-01-20T01:46:36Z

control/modelsimp.py

+        By default for continuous-time systems, alpha <= 0 defines the stability
+        boundary for the real part of A's eigenvalues and for discrete-time
+        systems, 0 <= alpha <= 1 defines the stability boundary for the modulus
+        of A's eigenvalues.See SLICOT routines AB09MD and AB09ND for more


While resolving the merge conflict, I added a space here after the .. So, now it reads "...eigenvalues. See SLICOT...".

slivingston · 2017-01-20T01:58:36Z

Your updates are good. Thanks for addressing my comments.

#118 Changes are from branch `master` of https://github.com/mdclemen/python-control.git

testBalredMatchDC, testGramRc, testGramRo which were introduced in PR #118 use functions that depend on Slycot, so the tests should be skipped if it is not found.

slivingston self-assigned this Dec 26, 2016

slivingston reviewed Dec 26, 2016

View reviewed changes

balred() now has 'matchdc' option and may handle unstable systems and…

9d840b8

… will accept a list of 'orders'. gram() now does Cholesky factored gramians if desired. Requires slycot routines AB09MD, AB09ND, SB03OD.

slivingston reviewed Jan 18, 2017

View reviewed changes

Updated docstring for balred to remove abbreviations and better descr…

4815cf0

…ibe `alpha`. In balred(), if method='truncate' and the system is stable, slycot ab09ab is used, otherwise ab09md is used.

slivingston reviewed Jan 20, 2017

View reviewed changes

slivingston merged commit 4815cf0 into python-control:master Jan 20, 2017

slivingston added a commit that referenced this pull request Jan 20, 2017

Merge pull request #118 "Updates to balred() and gram()"

1dcc842

#118 Changes are from branch `master` of https://github.com/mdclemen/python-control.git

mdclemen mentioned this pull request Jan 27, 2017

Truncated balanced realization for unstable state space models #77

Closed

slivingston mentioned this pull request Feb 17, 2017

Add run of tests without Slycot on Travis CI #133

Merged

murrayrm added this to the 0.8.0 milestone Dec 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates to balred() and gram() #118

Updates to balred() and gram() #118

mdclemen commented Oct 21, 2016

coveralls commented Dec 25, 2016 •

edited

Loading

slivingston Dec 26, 2016

mdclemen Dec 27, 2016

slivingston Dec 28, 2016

slivingston Dec 26, 2016

mdclemen Dec 27, 2016

slivingston Dec 28, 2016

slivingston commented Dec 28, 2016

slivingston commented Dec 28, 2016

mdclemen commented Jan 3, 2017

slivingston commented Jan 6, 2017

slivingston Jan 18, 2017

slivingston Jan 18, 2017

slivingston Jan 18, 2017

mdclemen Jan 18, 2017

slivingston Jan 18, 2017

slivingston Jan 18, 2017

mdclemen commented Jan 18, 2017 via email

slivingston commented Jan 18, 2017

slivingston commented Jan 18, 2017

slivingston commented Jan 18, 2017 •

edited

Loading

mdclemen commented Jan 19, 2017

mdclemen commented Jan 19, 2017

slivingston Jan 20, 2017

slivingston commented Jan 20, 2017

Updates to balred() and gram() #118

Updates to balred() and gram() #118

Conversation

mdclemen commented Oct 21, 2016

coveralls commented Dec 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slivingston commented Dec 28, 2016

slivingston commented Dec 28, 2016

mdclemen commented Jan 3, 2017

slivingston commented Jan 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdclemen commented Jan 18, 2017 via email

slivingston commented Jan 18, 2017

slivingston commented Jan 18, 2017

slivingston commented Jan 18, 2017 • edited Loading

mdclemen commented Jan 19, 2017

mdclemen commented Jan 19, 2017

Choose a reason for hiding this comment

slivingston commented Jan 20, 2017

coveralls commented Dec 25, 2016 •

edited

Loading

slivingston commented Jan 18, 2017 •

edited

Loading