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 — a first implementation of the singular value plot as discussed in #592 by forgi86 · Pull Request #593 · python-control/python-control

Conversation

@forgi86

@forgi86 forgi86 commented Mar 27, 2021

Copy link
Copy Markdown
Contributor
  • added singular values plot (function singular_values_plot in freqplot.py)
  • added a test of the new function (test_singular_values_plot in freqresp_test.py)
  • added an example jupyter notebook (singular-values-plot.ipynb)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@forgi86
DEV:
43e3779 
- added singular values plot (function singular_values_plot in freqplot.py)
- added a test of the new function (test_singular_values_plot  in freqresp_test.py)
- added an example jupyter notebook (singular-values-plot.ipynb)
bnavigator
bnavigator reviewed Mar 28, 2021
View reviewed changes

@bnavigator bnavigator 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.

Awesome!

I left some comments in the code.

The Notebook looks like omega exceeds the Nyquist frequency

Sorry, something went wrong.

Uh oh!

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

Uh oh!

There was an error while loading. Please reload this page.
Comment thread control/freqplot.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread control/freqplot.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread control/freqplot.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread control/freqplot.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread control/freqplot.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread control/freqplot.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread control/freqplot.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread control/tests/freqresp_test.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread control/tests/freqresp_test.py Resolved

Uh oh!

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

bnavigator commented Mar 28, 2021

Copy link
Copy Markdown
Contributor

The test suite failure is unrelated and will be fixed by #588
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@forgi86 @bnavigator
Update control/freqplot.py
ae7d546 
Co-authored-by: Ben Greiner <[email protected]>
@coveralls

coveralls commented Mar 28, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.1%) to 89.602% when pulling 649c394 on forgi86:singular-values-plot into a6a5bee on python-control:master.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
forgi86 and others added 3 commits March 28, 2021 13:05
@forgi86 @bnavigator
Update control/freqplot.py
36e8b77 
Co-authored-by: Ben Greiner <[email protected]>
@forgi86
FIX:
4e055bb 
- removed deprecated handling 'Plot'
- fixed docstring for argument plot the singular value plot
@forgi86
Merge remote-tracking branch 'origin/singular-values-plot' into singu…
14ef194 
…lar-values-plot
@bnavigator

bnavigator commented Mar 28, 2021

Copy link
Copy Markdown
Contributor 
            fresp = sys(1j*omega if sys.isctime() else np.exp(1j * omega * sys.dt))
    
>           fresp = fresp.transpose((2, 0, 1))
E           ValueError: axes don't match array

use squeeze=False
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@forgi86
FIX:
681edb7 
- added default settings for singular_values_plot
@forgi86

forgi86 commented Mar 28, 2021

Copy link
Copy Markdown
Contributor Author

Something strange about the return shape of the frequency response. For a square 2x2 system:

sys(np.array(0.0), squeeze=False).shape
(2, 2)

sys(0.0, squeeze=False).shape
(2, 2, 1)

Is it expected? Perhaps I should do an np.atleast_1d(omega) inside singular_values_plot anyways?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bnavigator bnavigator mentioned this pull request Mar 28, 2021
squeeze: return shape of the frequency response. #594
Closed
@bnavigator

bnavigator commented Mar 28, 2021

Copy link
Copy Markdown
Contributor

Is it expected? Perhaps I should do an np.atleast_1d(omega) inside singular_values_plot anyways?

Nope. Opened #594.

You can work around with atleast_1d.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
forgi86 added 2 commits March 28, 2021 23:13
@forgi86
FIX:
8d02fae 
- added np.atleast_1d(omega) to handle scalar omega parameter
@forgi86
FIX:
fe97069 
- reshape output of frequency response to handle MIMO/SISO systems in the same way
- if plot condition added to set gridlines and axes labels
bnavigator
bnavigator reviewed Mar 30, 2021
View reviewed changes
Comment thread control/freqplot.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
forgi86 added 2 commits March 31, 2021 15:04
@forgi86
FIX: development of parametrized tests for the singular_values_plot f…
762fbd6 
…unction
@forgi86
FIX: using option squeeze=False to keep all dimensions from the frequ…
3b09ae7 
…ency response in freqplot.py
bnavigator
bnavigator requested changes Mar 31, 2021
View reviewed changes
Comment thread control/freqplot.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread control/freqplot.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread control/tests/freqresp_test.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread control/tests/freqresp_test.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread control/tests/freqresp_test.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
forgi86 added 3 commits March 31, 2021 20:55
@forgi86
FIX:
9c68700 
- in singular_values_plot: the complex argument to be evaluated for the frequency response is now computed in the if branch
- fixed typos in freqresp_test.py: the siso system is now also tested
@forgi86
DOC:
a392c97 
- added documentation for parameters Hz and dB of singular_values_plot
@forgi86
FIX:
caa3e2c 
- result of singular_values_plot transposed to be in line with the "channel first" format
- tests also updated consequently
bnavigator
bnavigator reviewed Mar 31, 2021
View reviewed changes
Comment thread control/freqplot.py Resolved

Uh oh!

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

bnavigator commented Mar 31, 2021

Copy link
Copy Markdown
Contributor

Whishlist:
Can we (optionally) have the left and right singular vectors, too?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@forgi86
FIX:
0607314 
- use omega_sys to compute omega_complex!
@forgi86

forgi86 commented Mar 31, 2021

Copy link
Copy Markdown
Contributor Author

Whishlist:
Can we (optionally) have the left and right singular vectors, too?

Sounds reasonable. From a control perspective they correspond to the output/input directions associated with the corresponding singular value. There's no way to plot them in a nice way, but they can indeed be optionally returned.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
forgi86 added 3 commits April 1, 2021 01:14
@forgi86
TST:
65b4e49 
- added discrete-time example
- added tests with more than one frequency point
@forgi86
TST:
87d48ab 
- added a test that generates a plot
@forgi86
MAINT: refactoring of the frequency selection logic as a private func…
a289f8c 
…tion used by bode_plot and singular_values_plot
bnavigator
bnavigator reviewed Apr 2, 2021
View reviewed changes
Comment thread control/tests/freqresp_test.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
murrayrm
murrayrm reviewed Apr 3, 2021
View reviewed changes
Comment thread control/freqplot.py Outdated
Comment on lines +1031 to +1034
'singular_values_plot.dB': False, # Plot singular values in dB
'singular_values_plot.deg': True, # Plot phase in degrees
'singular_values_plot.Hz': False, # Plot frequency in Hertz
'singular_values_plot.grid': True, # Turn on grid for gain and phase

@murrayrm murrayrm Apr 3, 2021

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.

It seems a bit odd to me to have to set these separately from the equivalent bode configuration variable. Is someone going to use one set of defaults for Bode and a separate one for singular values? (Also note that gang_of_four uses the Bode plot defaults.)

If we don't want to use bode.dB, etc, we could also change all of them to freqplot.dB (though perhaps with some backward compatibility??).

Sorry, something went wrong.

Uh oh!

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

@bnavigator bnavigator Apr 3, 2021

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.

#593 (comment)

Sorry, something went wrong.

Uh oh!

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

@forgi86 forgi86 Apr 3, 2021

Copy link
Copy Markdown
Contributor Author

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'd add the properties dB, Hz, and grid to freqplot and use these for bode, singular_values_plot, and gang_of_four.
The property _singular_values_plot_default would then disappear, and _bode_defaults would still contain bode.deg and bode.wrap_phase (and maybe unused dB, Hz, grid for backward compatibility, at least for the moment).

Is this OK?

Sorry, something went wrong.

Uh oh!

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

@murrayrm murrayrm Apr 3, 2021

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.

That seems fine. Make sure also to update the use_matlab_defaults and use_fbs_defaults functions in config.py. I'm not sure about what to do with bode.deg. On the one hand, it is true that this is the only function that plots phase, but at the same time seems odd to use freqplot for everything except that. I would probably just use freqplot for everything.

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

@forgi86 forgi86 Apr 7, 2021

Copy link
Copy Markdown
Contributor Author

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 pushed a new version where all the settings for bode_plot and singular_values_plot are in _freqplot_defaults

Sorry, something went wrong.

Uh oh!

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

@bnavigator bnavigator Apr 7, 2021

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 still see a _singular_values_plot_default with a comment for "Bode" above the definition and then used with_get_param('freqplot'). That's very confusing.

Also, personally, I would still prefer that we provide some backwards compatibility through an alias of config.defaults['bode'], so that old code does not break right away. Maybe including a warning message. I am not sure how to achieve this with a dict outside of set_defaults and _get_param in an elegant way.

Sorry, something went wrong.

Uh oh!

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

@forgi86 forgi86 Apr 7, 2021

Copy link
Copy Markdown
Contributor Author

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 tried to remove all references to the previous structure _singular_values_plot_default (new push).
Still have to handle backward compatibility in some way.

Sorry, something went wrong.

Uh oh!

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

Uh oh!

There was an error while loading. Please reload this page.
Comment thread examples/singular-values-plot.ipynb Resolved

Uh oh!

There was an error while loading. Please reload this page.
forgi86 added 5 commits April 3, 2021 15:10
@forgi86
MAINT:
7d19b30 
- changed line plot = config._get_param('bode', 'grid', plot, True) to config._get_param('bode', 'grid', plot, True)
- changed logic in _determine_omega_vector: if Hz = True, omega_in is interpreted in Hz units (as it is with omega_limits)
- jupyter notebook singular-values-plot.ipynb output fixed
@forgi86
DOC: updated example for singular_values_plot
34c3826 
DEV: improved color cycling logic for superimposed singular_values_plot on the same axes: do not repeat the same color
@forgi86
MAINT: _bode_defaults removed, fields moved in _freqplot_default
48015d5 
TST: tests in config_test modified to interpret omega in Hz if the option Hz is set to True
@forgi86
MAINT: no warning issued for evaluation above the Nyquist frequency
38dfeda
@forgi86
MAINT: removing references to singular_values_plot configurations
555902b
bnavigator
bnavigator reviewed Apr 7, 2021
View reviewed changes
Comment thread control/freqplot.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
forgi86 and others added 3 commits April 7, 2021 19:25
@forgi86
MAINT: removing references to singular_values_plot configurations
ec69337
@forgi86
MAINT: removing references to bode configurations (use freqplot instead)
288a7ee
@forgi86 @bnavigator
Update control/freqplot.py
8f3f76d 
Co-authored-by: Ben Greiner <[email protected]>
@bnavigator

bnavigator commented Apr 11, 2021

Copy link
Copy Markdown
Contributor

https://github.com/python-control/python-control/blob/8f3f76debd0cb95725d4070a29c158f6698ea6b8/doc/conventions.rst#package-configuration-parameters needs an update.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
forgi86 and others added 4 commits April 18, 2021 21:13
@forgi86
MAINT: the Hz parameter of bode_plot and singular_values_plot now onl…
fb3d9a0 
…y affects the plot (input/outputs are always in rad/sec)
@forgi86
DOC: bode changed to freqplot in conventions.rst
37c0962
@forgi86
Merge remote-tracking branch 'origin/singular-values-plot' into singu…
a486c18 
…lar-values-plot
@forgi86
Merge branch 'master' into singular-values-plot
9143d75
@forgi86

forgi86 commented Apr 18, 2021

Copy link
Copy Markdown
Contributor Author

Variables renamed in conventions.rst (bode -> freqplot)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sawyerbfuller
sawyerbfuller approved these changes Apr 20, 2021
View reviewed changes
@sawyerbfuller sawyerbfuller linked an issue Apr 20, 2021 that may be closed by this pull request
Singular values plot missing #592
Closed
bnavigator
bnavigator reviewed Apr 20, 2021
View reviewed changes
Comment thread control/freqplot.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
forgi86 added 3 commits April 23, 2021 23:32
@forgi86
DOC: modified docstring (Examples section) in singular_values_plot to…
f5de343 
… make it doctest compliant
@forgi86
DOC: modified docstring (Examples section) in singular_values_plot to…
9638e92 
… make it doctest compliant
@forgi86
DOC: modified docstring (Examples section) in singular_values_plot to…
10cde40 
… make it doctest compliant
bnavigator
bnavigator reviewed Apr 23, 2021
View reviewed changes
Comment thread control/freqplot.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
forgi86 and others added 2 commits April 24, 2021 09:56
@forgi86
DOC: modified imports in docstring (Examples section) in singular_val…
0ada3f6 
…ues_plot
@bnavigator
Merge branch 'master' into pr-593
649c394
@bnavigator

bnavigator commented Apr 29, 2021

Copy link
Copy Markdown
Contributor

Thank you @forgi86 for the hard work and also for the patience with all the picky reviews.

I will submit a PR shortly which addresses the backwards compatibility with the old name for the bode defaults.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bnavigator
bnavigator merged commit db174b7 into python-control:master Apr 29, 2021
@bnavigator bnavigator mentioned this pull request Apr 29, 2021
add DefaultDict for deprecation handling #619
Merged
@forgi86

forgi86 commented May 15, 2021

Copy link
Copy Markdown
Contributor Author

Thank you for the review and for your kind message, @bnavigator. Was my first PR to python-control, I hope I will be able to contribute again in the future!
bnavigator 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.
@murrayrm murrayrm added this to the 0.9.1 milestone Dec 30, 2021
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
@bnavigator bnavigator bnavigator requested changes
@sawyerbfuller sawyerbfuller sawyerbfuller approved these changes

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.1

Development

Successfully merging this pull request may close these issues.

Singular values plot missing

Uh oh!

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

5 participants

@forgi86 @bnavigator @coveralls @murrayrm @sawyerbfuller
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.