Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Feature print zpk#869

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
murrayrm merged 16 commits intopython-control:mainfromhenklaak:feature_print_zpk
Mar 25, 2023
Merged

Feature print zpk#869

murrayrm merged 16 commits intopython-control:mainfromhenklaak:feature_print_zpk
Mar 25, 2023

Conversation

henklaak
Copy link
Contributor

@henklaakhenklaak commentedFeb 21, 2023•
edited
Loading

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 + 12 (s + 1) (s + 2)----------------- (s - 1) (s - 1)2 z^2 + 6 z + 4--------------- z^2 - 2 z + 1dt = 0.12 (z + 1) (z + 2)----------------- (z - 1) (z - 1)dt = 0.1

@coveralls
Copy link

coveralls commentedFeb 21, 2023•
edited
Loading

Coverage Status

Coverage: 94.841% (-0.04%) from 94.883% when pulling6933973 on henklaak:feature_print_zpk into08dcc95 on python-control:main.

Copy link
Contributor

@sawyerbfullersawyerbfuller left a 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’stf2zpk. 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 likezpkdata 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.

@henklaak
Copy link
ContributorAuthor

henklaak commentedFeb 23, 2023•
edited
Loading

@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
Copy link
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 withzpk. And output accordingly in response to that flag in_repr_latex?

@henklaak
Copy link
ContributorAuthor

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

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

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

Choose a reason for hiding this comment

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

Suggested change
#Seeif this is a discretetime system withspecific samplingtime
#print value of dt onlyif this is a discrete-time system withdt not 'True' (unspecified sampligntime)

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

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',    }

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

Choose a reason for hiding this comment

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

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

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

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'

if display_format is None:
display_format = 'poly'

if display_format not in ('poly', 'zpk'):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ifdisplay_formatnotin ('poly','zpk'):
ifself.display_formatnotin ('poly','zpk'):


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

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)

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

self.display_format = display_format
Copy link
Contributor

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

@@ -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):
Copy link
Contributor

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):

@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Defaultbehaviorispolynomialdisplay.
Defaultbehaviorispolynomialdisplayandcanbechangedby
changingconfig.defaults['xferfcn.display_format'].

@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Defaultbehaviorispolynomialdisplay.
Defaultbehaviorispolynomialdisplayandcanbechangedby
changingconfig.defaults['xferfcn.display_format'].

@henklaak
Copy link
ContributorAuthor

henklaak commentedFeb 25, 2023•
edited
Loading

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

@sawyerbfuller
Copy link
Contributor

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

@sawyerbfuller
Copy link
Contributor

display_num_format?

@henklaak
Copy link
ContributorAuthor

henklaak commentedFeb 25, 2023•
edited
Loading

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.

'xferfcn.floating_point_format': '.4g'
}

def _float2str(value):

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}}")Bla0.232655

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

sawyerbfuller reacted with thumbs up emoji
Copy link
Member

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.

@sawyerbfuller
Copy link
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 oneprint_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
Copy link
ContributorAuthor

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

@murrayrmmurrayrm merged commit26c44e1 intopython-control:mainMar 25, 2023
@henklaak
Copy link
ContributorAuthor

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

@murrayrmmurrayrm added this to the0.9.4 milestoneMar 27, 2023
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@murrayrmmurrayrmmurrayrm left review comments

@ilaynilaynilayn left review comments

@sawyerbfullersawyerbfullersawyerbfuller approved these changes

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
5 participants
@henklaak@coveralls@sawyerbfuller@murrayrm@ilayn
[8]ページ先頭
©2009-2025 Movatter.jp