Feature print zpk #869

henklaak · 2023-02-21T16:09:45Z

Fixes #64

G = tf([2, 6, 4],[1,-2,1], display_format='poly')
print(G)

G = tf([2, 6, 4],[1,-2,1], display_format='zpk')
print(G)

G = tf([2, 6, 4],[1,-2,1], dt=0.1, display_format='poly')
print(G)

G = tf([2, 6, 4],[1,-2,1], dt=0.1, display_format='zpk')
print(G)

Produces:


2 s^2 + 6 s + 4
---------------
 s^2 - 2 s + 1


2 (s + 1) (s + 2)
-----------------
 (s - 1) (s - 1)


2 z^2 + 6 z + 4
---------------
 z^2 - 2 z + 1

dt = 0.1


2 (z + 1) (z + 2)
-----------------
 (z - 1) (z - 1)

dt = 0.1

coveralls · 2023-02-21T16:25:46Z

Coverage: 94.841% (-0.04%) from 94.883% when pulling 6933973 on henklaak:feature_print_zpk into 08dcc95 on python-control:main.

control/xferfcn.py

sawyerbfuller

This would be a very nice feature. I am not sure there are any plans to have a new object class that stores things in zpk form directly, so having convenient methods like this to access that data seems to me like the way to go.

A few suggestions/thoughts:

might be simpler and more compact on lines 497ish to 523ish to use scipy’s tf2zpk. And maybe more robust? Unless your code does it better than how scipy does it.
consider refactoring to have this use an intermediate method called something like zpkdata that serves as a direct way to access zpk numerical values.
In the nice-to-have category would be automatic latex representation when you are in a Jupyter notebook, like how tf and ss systems do it.

control/xferfcn.py

henklaak · 2023-02-23T21:34:04Z

@sawyerbfuller Thanks for the review!

The nice-to-have latex would be difficult, since Jupyter eventually calls _repr_latex_ which is currently used for regular polynomial printing.
These is no obvious way (except introducing a new default boolean) to switch between full polynomial / factorized printing. Personally I am not a big fan of the defaults mechanism, as this is hidden from the public API.

Maybe a specific subclass for ZPK, but that would be an invasive change. Let me mull it over. Otherwise it will have to stay nice-to-have ;-)

I'll have a look to see what's possible/convenient for the other suggestions/thoughts.

sawyerbfuller · 2023-02-24T03:32:09Z

@henklaak one possibility that might not break anything might be to add a field to TransferFunction, maybe ‘representation’?, that could be set to either ‘poly’ or ‘zpk’ in init. Might be automatically set to ‘zpk’ if instantiated with zpk. And output accordingly in response to that flag in _repr_latex?

henklaak · 2023-02-24T10:03:45Z

Thanks for the invaluable comments. The PR has become so much better now.
I had completely overlooked the tf2zpk functionality in SciPy :-(

sawyerbfuller · 2023-02-24T16:58:37Z

control/xferfcn.py

@@ -463,8 +483,7 @@ def __str__(self, var=None):

        # See if this is a discrete time system with specific sampling time


Suggested change

# See if this is a discrete time system with specific sampling time

# print value of dt only if this is a discrete-time system with dt not 'True' (unspecified samplign time)

sawyerbfuller · 2023-02-24T17:04:00Z

control/xferfcn.py

@@ -92,6 +92,9 @@ class TransferFunction(LTI):
        time, positive number is discrete time with specified


I can't suggest changes above this line so I am suggsting them here:

_xferfcn_defaults = { 'xferfcn.display_format': 'poly', }

sawyerbfuller · 2023-02-24T17:06:22Z

control/xferfcn.py

@@ -198,6 +201,14 @@ def __init__(self, *args, **kwargs):
        #
        # Process keyword arguments
        #
+        if display_format is None:


Suggested change

if display_format is None:

self.display_format = kwargs.pop('display_format',

config.defaults['xferfcn.display_format'])

sawyerbfuller · 2023-02-24T17:06:35Z

control/xferfcn.py

@@ -198,6 +201,14 @@ def __init__(self, *args, **kwargs):
        #
        # Process keyword arguments
        #
+        if display_format is None:
+            display_format = 'poly'


Suggested change

display_format = 'poly'

sawyerbfuller · 2023-02-24T17:07:32Z

control/xferfcn.py

+        if display_format is None:
+            display_format = 'poly'
+
+        if display_format not in ('poly', 'zpk'):


Suggested change

if display_format not in ('poly', 'zpk'):

if self.display_format not in ('poly', 'zpk'):

sawyerbfuller · 2023-02-24T17:07:48Z

control/xferfcn.py

+
+        if display_format not in ('poly', 'zpk'):
+            raise ValueError("display_format must be 'poly' or 'zpk',"
+                             " got '%s'" % display_format)


Suggested change

" got '%s'" % display_format)

" got '%s'" % self.display_format)

sawyerbfuller · 2023-02-24T17:07:58Z

control/xferfcn.py

+            raise ValueError("display_format must be 'poly' or 'zpk',"
+                             " got '%s'" % display_format)
+
+        self.display_format = display_format


Suggested change

self.display_format = display_format

sawyerbfuller · 2023-02-24T17:08:34Z

control/xferfcn.py

@@ -149,7 +152,7 @@ class TransferFunction(LTI):
    # Give TransferFunction._rmul_() priority for ndarray * TransferFunction
    __array_priority__ = 11     # override ndarray and matrix types

-    def __init__(self, *args, **kwargs):
+    def __init__(self, *args, display_format=None, **kwargs):


Suggested change

def __init__(self, *args, display_format=None, **kwargs):

def __init__(self, *args, **kwargs):

sawyerbfuller · 2023-02-24T17:12:10Z

control/xferfcn.py

@@ -92,6 +92,9 @@ class TransferFunction(LTI):
        time, positive number is discrete time with specified
        sampling time, None indicates unspecified timebase (either
        continuous or discrete time).
+    display_format: None, 'poly' or 'zpk'
+        Set the display format used in printing the TransferFunction object.
+        Default behavior is polynomial display.


Suggested change

Default behavior is polynomial display.

Default behavior is polynomial display and can be changed by

changing config.defaults['xferfcn.display_format'].

sawyerbfuller · 2023-02-24T17:21:32Z

control/xferfcn.py

@@ -1486,6 +1552,9 @@ def tf(*args, **kwargs):
        Polynomial coefficients of the numerator
    den: array_like, or list of list of array_like
        Polynomial coefficients of the denominator
+    display_format: None, 'poly' or 'zpk'
+        Set the display format used in printing the TransferFunction object.
+        Default behavior is polynomial display.


Suggested change

Default behavior is polynomial display.

Default behavior is polynomial display and can be changed by

changing config.defaults['xferfcn.display_format'].

henklaak · 2023-02-25T11:47:30Z

I think we have solved this quite nicely.

-edit- On the other hand, now we have defaults, we could also make the floating point format (currently '%.4g') a configurable default.

-edit- Couldn't resist. Added it.

sawyerbfuller · 2023-02-25T14:52:22Z

In statesp the number format option is ’statesp.latex_num_format': '.3g' Maybe we should standardize on a common name?

sawyerbfuller · 2023-02-25T14:55:14Z

display_num_format?

henklaak · 2023-02-25T14:56:37Z

I considered that, but the screen space requirements that lead to the format specifiers are rather unrelated.
How about we work on this in a new issue and PR and discuss it there? Can be done quickly from my side.

There are actually a lot of places where one would like to customize the precision, e.g. also in plots.
I think it's hard to get to a 'one size fits all' default.

ilayn · 2023-02-25T16:25:48Z

control/xferfcn.py

+    'xferfcn.floating_point_format': '.4g'
+}
+
+def _float2str(value):


I would suggest against doing this. It's anti-pattern for Python and also there is a simpler way to grab the default at the code level and reuse.

>>> _num_format = config.defaults.get('xferfcn.floating_point_format', ':.4g') >>> print(f"Bla {0.232654684684654654:{_num_format}}") Bla 0.232655

So you don't need to inject f-string machinery to the string representation.

@henklaak If you can fix up this final issue, I think this PR is ready to post.

sawyerbfuller · 2023-02-25T16:28:42Z

@henklaak re display format for text vs latex: makes sense I agree they have different requirements, if I understand what you’re saying. In that case we should have setttings for both latex and text.

Should they then have otherwise consistent names with only “latex” vs “print” identifiers that distinguish? That would suggest making this one print_num_format?

if you think it should have a different name then hapoy to leave that discussion to a different PR.

Otherwise this looks good to me

henklaak · 2023-03-01T17:44:22Z

Down with the flu. Will continue end of the week.

henklaak · 2023-03-27T00:08:28Z

Thanks @murrayrm for taking this one.
Some serious family health issues, I've been a bit low on motivation/interest.
H.

henklaak added 4 commits February 21, 2023 16:26

Add print zpk form

5c85459

Fix unittest

57620f1

Add fix for precision noise

b36732d

Cleanup unittest

45b5ae4

sawyerbfuller reviewed Feb 23, 2023

View reviewed changes

control/xferfcn.py Outdated Show resolved Hide resolved

sawyerbfuller reviewed Feb 23, 2023

View reviewed changes

control/xferfcn.py Outdated Show resolved Hide resolved

sawyerbfuller reviewed Feb 23, 2023

View reviewed changes

control/xferfcn.py Outdated Show resolved Hide resolved

sawyerbfuller reviewed Feb 23, 2023

View reviewed changes

control/xferfcn.py Show resolved Hide resolved

henklaak and others added 2 commits February 24, 2023 10:01

Merge branch 'python-control:main' into feature_print_zpk

07a859d

Rework after review

93f7b46

henklaak added 4 commits February 24, 2023 11:05

Shouldn't have commited the pyproject.toml, revering

d2807b0

Fix variable mixup due to inconsistent naming

5652bf0

Fix variable mixup due to inconsistent naming

58e9d63

Typos

521a306

sawyerbfuller approved these changes Feb 24, 2023

View reviewed changes

henklaak added 2 commits February 25, 2023 12:35

Add default for xferfcn.display_format

2644874

Update after failed tests

8101b31

henklaak added 2 commits February 25, 2023 13:48

Add default for xferfcn.floating_point_format

8d7ac56

Fix improper way of setting defaults in test

2ca47e1

ilayn reviewed Feb 25, 2023

View reviewed changes

henklaak and others added 2 commits March 5, 2023 13:28

Merge branch 'python-control:main' into feature_print_zpk

5f506b2

update xferfcn._float2str to be more pythonic per @ilayn

6933973

murrayrm merged commit 26c44e1 into python-control:main Mar 25, 2023

murrayrm added this to the 0.9.4 milestone Mar 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature print zpk #869

Feature print zpk #869

henklaak commented Feb 21, 2023 •

edited

Loading

coveralls commented Feb 21, 2023 •

edited

Loading

sawyerbfuller left a comment

henklaak commented Feb 23, 2023 •

edited

Loading

sawyerbfuller commented Feb 24, 2023

henklaak commented Feb 24, 2023

sawyerbfuller Feb 24, 2023

sawyerbfuller Feb 24, 2023

sawyerbfuller Feb 24, 2023

sawyerbfuller Feb 24, 2023

sawyerbfuller Feb 24, 2023

sawyerbfuller Feb 24, 2023

sawyerbfuller Feb 24, 2023

sawyerbfuller Feb 24, 2023

sawyerbfuller Feb 24, 2023

sawyerbfuller Feb 24, 2023

henklaak commented Feb 25, 2023 •

edited

Loading

sawyerbfuller commented Feb 25, 2023

sawyerbfuller commented Feb 25, 2023

henklaak commented Feb 25, 2023 •

edited

Loading

ilayn Feb 25, 2023

murrayrm Mar 24, 2023

sawyerbfuller commented Feb 25, 2023

henklaak commented Mar 1, 2023

henklaak commented Mar 27, 2023

		@@ -463,8 +483,7 @@ def __str__(self, var=None):

		# See if this is a discrete time system with specific sampling time

	# See if this is a discrete time system with specific sampling time
	# print value of dt only if this is a discrete-time system with dt not 'True' (unspecified samplign time)

		@@ -92,6 +92,9 @@ class TransferFunction(LTI):
		time, positive number is discrete time with specified

	if display_format is None:
	self.display_format = kwargs.pop('display_format',
	config.defaults['xferfcn.display_format'])

	if display_format not in ('poly', 'zpk'):
	if self.display_format not in ('poly', 'zpk'):

	" got '%s'" % display_format)
	" got '%s'" % self.display_format)

	def __init__(self, args, display_format=None, *kwargs):
	def __init__(self, args, *kwargs):

	Default behavior is polynomial display.
	Default behavior is polynomial display and can be changed by
	changing config.defaults['xferfcn.display_format'].

Feature print zpk #869

Feature print zpk #869

Conversation

henklaak commented Feb 21, 2023 • edited Loading

coveralls commented Feb 21, 2023 • edited Loading

sawyerbfuller left a comment

Choose a reason for hiding this comment

henklaak commented Feb 23, 2023 • edited Loading

sawyerbfuller commented Feb 24, 2023

henklaak commented Feb 24, 2023

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

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

henklaak commented Feb 25, 2023 • edited Loading

sawyerbfuller commented Feb 25, 2023

sawyerbfuller commented Feb 25, 2023

henklaak commented Feb 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sawyerbfuller commented Feb 25, 2023

henklaak commented Mar 1, 2023

henklaak commented Mar 27, 2023

henklaak commented Feb 21, 2023 •

edited

Loading

coveralls commented Feb 21, 2023 •

edited

Loading

henklaak commented Feb 23, 2023 •

edited

Loading

henklaak commented Feb 25, 2023 •

edited

Loading

henklaak commented Feb 25, 2023 •

edited

Loading