GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Feature print zpk by henklaak · Pull Request #869 · python-control/python-control

Conversation

@henklaak

@henklaak henklaak commented Feb 21, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
henklaak added 4 commits February 21, 2023 16:26
@henklaak
Add print zpk form
5c85459
@henklaak
Fix unittest
57620f1
@henklaak
Add fix for precision noise
b36732d
@henklaak
Cleanup unittest
45b5ae4
@coveralls

coveralls commented Feb 21, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Coverage Status

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sawyerbfuller
sawyerbfuller reviewed Feb 23, 2023
View reviewed changes
Comment thread control/xferfcn.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
sawyerbfuller
sawyerbfuller reviewed Feb 23, 2023
View reviewed changes
Comment thread control/xferfcn.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
sawyerbfuller
sawyerbfuller reviewed Feb 23, 2023
View reviewed changes
Comment thread control/xferfcn.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
sawyerbfuller
sawyerbfuller reviewed Feb 23, 2023
View reviewed changes

@sawyerbfuller sawyerbfuller left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
sawyerbfuller
sawyerbfuller reviewed Feb 23, 2023
View reviewed changes
Comment thread control/xferfcn.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@henklaak

henklaak commented Feb 23, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sawyerbfuller

sawyerbfuller commented Feb 24, 2023

Copy link
Copy Markdown
Contributor

@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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
henklaak and others added 2 commits February 24, 2023 10:01
@henklaak
Merge branch 'python-control:main' into feature_print_zpk
07a859d
@henklaak
Rework after review
93f7b46
@henklaak

henklaak commented Feb 24, 2023

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
henklaak added 4 commits February 24, 2023 11:05
@henklaak
Shouldn't have commited the pyproject.toml, revering
d2807b0
@henklaak
Fix variable mixup due to inconsistent naming
5652bf0
@henklaak
Fix variable mixup due to inconsistent naming
58e9d63
@henklaak
Typos
521a306
sawyerbfuller
sawyerbfuller approved these changes Feb 24, 2023
View reviewed changes
Comment thread control/xferfcn.py Outdated
@@ -463,8 +483,7 @@ def __str__(self, var=None):

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

@sawyerbfuller sawyerbfuller Feb 24, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread control/xferfcn.py
@@ -92,6 +92,9 @@ class TransferFunction(LTI):
time, positive number is discrete time with specified

@sawyerbfuller sawyerbfuller Feb 24, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread control/xferfcn.py Outdated
#
# Process keyword arguments
#
if display_format is None:

@sawyerbfuller sawyerbfuller Feb 24, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread control/xferfcn.py Outdated
# Process keyword arguments
#
if display_format is None:
display_format = 'poly'

@sawyerbfuller sawyerbfuller Feb 24, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
display_format = 'poly'

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread control/xferfcn.py Outdated
if display_format is None:
display_format = 'poly'

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

@sawyerbfuller sawyerbfuller Feb 24, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread control/xferfcn.py Outdated

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

@sawyerbfuller sawyerbfuller Feb 24, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread control/xferfcn.py Outdated
raise ValueError("display_format must be 'poly' or 'zpk',"
" got '%s'" % display_format)

self.display_format = display_format

@sawyerbfuller sawyerbfuller Feb 24, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self.display_format = display_format

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread control/xferfcn.py Outdated
__array_priority__ = 11 # override ndarray and matrix types

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

@sawyerbfuller sawyerbfuller Feb 24, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread control/xferfcn.py Outdated
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.

@sawyerbfuller sawyerbfuller Feb 24, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread control/xferfcn.py Outdated
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.

@sawyerbfuller sawyerbfuller Feb 24, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
henklaak added 2 commits February 25, 2023 12:35
@henklaak
Add default for xferfcn.display_format
2644874
@henklaak
Update after failed tests
8101b31
@henklaak

henklaak commented Feb 25, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
henklaak added 2 commits February 25, 2023 13:48
@henklaak
Add default for xferfcn.floating_point_format
8d7ac56
@henklaak
Fix improper way of setting defaults in test
2ca47e1
@sawyerbfuller

sawyerbfuller commented Feb 25, 2023

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sawyerbfuller

sawyerbfuller commented Feb 25, 2023

Copy link
Copy Markdown
Contributor

display_num_format?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@henklaak

henklaak commented Feb 25, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ilayn
ilayn reviewed Feb 25, 2023
View reviewed changes
Comment thread control/xferfcn.py
'xferfcn.floating_point_format': '.4g'
}

def _float2str(value):

@ilayn ilayn Feb 25, 2023

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sawyerbfuller reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@murrayrm murrayrm Mar 24, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@sawyerbfuller

sawyerbfuller commented Feb 25, 2023

Copy link
Copy Markdown
Contributor

@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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@henklaak

henklaak commented Mar 1, 2023

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
henklaak and others added 2 commits March 5, 2023 13:28
@henklaak
Merge branch 'python-control:main' into feature_print_zpk
5f506b2
@murrayrm
update xferfcn._float2str to be more pythonic per @ilayn
6933973
@murrayrm
murrayrm merged commit 26c44e1 into python-control:main Mar 25, 2023
@henklaak

henklaak commented Mar 27, 2023

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@murrayrm murrayrm added this to the 0.9.4 milestone Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@murrayrm murrayrm murrayrm left review comments
@sawyerbfuller sawyerbfuller sawyerbfuller approved these changes
+1 more reviewer
@ilayn ilayn ilayn left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.9.4

Development

Successfully merging this pull request may close these issues.

Feature request pretty printing zpk form

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@henklaak @coveralls @sawyerbfuller @murrayrm @ilayn
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.