a first implementation of the singular value plot as discussed in #592 #593

forgi86 · 2021-03-27T23:11:10Z

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)

- 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

Awesome!

I left some comments in the code.

The Notebook looks like omega exceeds the Nyquist frequency

control/freqplot.py

control/tests/freqresp_test.py

bnavigator · 2021-03-28T10:15:12Z

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

Co-authored-by: Ben Greiner <code@bnavigator.de>

coveralls · 2021-03-28T10:58:22Z

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

Co-authored-by: Ben Greiner <code@bnavigator.de>

- removed deprecated handling 'Plot' - fixed docstring for argument plot the singular value plot

…lar-values-plot

bnavigator · 2021-03-28T12:48:26Z

            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

- added default settings for singular_values_plot

forgi86 · 2021-03-28T17:36:33Z

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?

bnavigator · 2021-03-28T19:00:44Z

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.

- added np.atleast_1d(omega) to handle scalar omega parameter

- reshape output of frequency response to handle MIMO/SISO systems in the same way - if plot condition added to set gridlines and axes labels

control/freqplot.py

…unction

…ency response in freqplot.py

control/freqplot.py

control/tests/freqresp_test.py

- 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

- added documentation for parameters Hz and dB of singular_values_plot

- result of singular_values_plot transposed to be in line with the "channel first" format - tests also updated consequently

control/freqplot.py

bnavigator · 2021-03-31T20:34:05Z

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

- use omega_sys to compute omega_complex!

forgi86 · 2021-03-31T22:12:33Z

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.

- added discrete-time example - added tests with more than one frequency point

- added a test that generates a plot

…tion used by bode_plot and singular_values_plot

control/tests/freqresp_test.py

murrayrm · 2021-04-03T02:20:24Z

control/freqplot.py

+    '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


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??).

#593 (comment)

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?

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.

I pushed a new version where all the settings for bode_plot and singular_values_plot are in _freqplot_defaults

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.

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.

control/freqplot.py

examples/singular-values-plot.ipynb

- 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

DEV: improved color cycling logic for superimposed singular_values_plot on the same axes: do not repeat the same color

TST: tests in config_test modified to interpret omega in Hz if the option Hz is set to True

control/freqplot.py

Co-authored-by: Ben Greiner <code@bnavigator.de>

bnavigator · 2021-04-11T14:49:19Z

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

…y affects the plot (input/outputs are always in rad/sec)

…lar-values-plot

forgi86 · 2021-04-18T19:48:29Z

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

control/freqplot.py

… make it doctest compliant

control/freqplot.py

…ues_plot

bnavigator · 2021-04-29T20:08:47Z

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.

forgi86 · 2021-05-15T21:08:31Z

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!

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 reviewed Mar 28, 2021

View reviewed changes

Update control/freqplot.py

ae7d546

Co-authored-by: Ben Greiner <code@bnavigator.de>

forgi86 and others added 3 commits March 28, 2021 13:05

Update control/freqplot.py

36e8b77

Co-authored-by: Ben Greiner <code@bnavigator.de>

FIX:

4e055bb

- removed deprecated handling 'Plot' - fixed docstring for argument plot the singular value plot

Merge remote-tracking branch 'origin/singular-values-plot' into singu…

14ef194

…lar-values-plot

FIX:

681edb7

- added default settings for singular_values_plot

bnavigator mentioned this pull request Mar 28, 2021

squeeze: return shape of the frequency response. #594

Closed

forgi86 added 2 commits March 28, 2021 23:13

FIX:

8d02fae

- added np.atleast_1d(omega) to handle scalar omega parameter

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 reviewed Mar 30, 2021

View reviewed changes

control/freqplot.py Outdated Show resolved Hide resolved

forgi86 added 2 commits March 31, 2021 15:04

FIX: development of parametrized tests for the singular_values_plot f…

762fbd6

…unction

FIX: using option squeeze=False to keep all dimensions from the frequ…

3b09ae7

…ency response in freqplot.py

bnavigator requested changes Mar 31, 2021

View reviewed changes

forgi86 added 3 commits March 31, 2021 20:55

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

DOC:

a392c97

- added documentation for parameters Hz and dB of singular_values_plot

FIX:

caa3e2c

- result of singular_values_plot transposed to be in line with the "channel first" format - tests also updated consequently

bnavigator reviewed Mar 31, 2021

View reviewed changes

control/freqplot.py Outdated Show resolved Hide resolved

FIX:

0607314

- use omega_sys to compute omega_complex!

forgi86 added 3 commits April 1, 2021 01:14

TST:

65b4e49

- added discrete-time example - added tests with more than one frequency point

TST:

87d48ab

- added a test that generates a plot

MAINT: refactoring of the frequency selection logic as a private func…

a289f8c

…tion used by bode_plot and singular_values_plot

bnavigator reviewed Apr 2, 2021

View reviewed changes

control/tests/freqresp_test.py Outdated Show resolved Hide resolved

murrayrm reviewed Apr 3, 2021

View reviewed changes

forgi86 added 5 commits April 3, 2021 15:10

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

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

MAINT: no warning issued for evaluation above the Nyquist frequency

38dfeda

MAINT: removing references to singular_values_plot configurations

555902b

bnavigator reviewed Apr 7, 2021

View reviewed changes

control/freqplot.py Outdated Show resolved Hide resolved

forgi86 and others added 3 commits April 7, 2021 19:25

MAINT: removing references to singular_values_plot configurations

ec69337

MAINT: removing references to bode configurations (use freqplot instead)

288a7ee

Update control/freqplot.py

8f3f76d

Co-authored-by: Ben Greiner <code@bnavigator.de>

forgi86 and others added 4 commits April 18, 2021 21:13

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)

DOC: bode changed to freqplot in conventions.rst

37c0962

Merge remote-tracking branch 'origin/singular-values-plot' into singu…

a486c18

…lar-values-plot

Merge branch 'master' into singular-values-plot

9143d75

sawyerbfuller approved these changes Apr 20, 2021

View reviewed changes

sawyerbfuller linked an issue Apr 20, 2021 that may be closed by this pull request

Singular values plot missing #592

Closed

bnavigator reviewed Apr 20, 2021

View reviewed changes

control/freqplot.py Outdated Show resolved Hide resolved

forgi86 added 3 commits April 23, 2021 23:32

DOC: modified docstring (Examples section) in singular_values_plot to…

f5de343

… make it doctest compliant

DOC: modified docstring (Examples section) in singular_values_plot to…

9638e92

… make it doctest compliant

DOC: modified docstring (Examples section) in singular_values_plot to…

10cde40

… make it doctest compliant

bnavigator reviewed Apr 23, 2021

View reviewed changes

control/freqplot.py Outdated Show resolved Hide resolved

forgi86 and others added 2 commits April 24, 2021 09:56

DOC: modified imports in docstring (Examples section) in singular_val…

0ada3f6

…ues_plot

Merge branch 'master' into pr-593

649c394

bnavigator merged commit db174b7 into python-control:master Apr 29, 2021

bnavigator mentioned this pull request Apr 29, 2021

add DefaultDict for deprecation handling #619

Merged

murrayrm added this to the 0.9.1 milestone Dec 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

a first implementation of the singular value plot as discussed in #592 #593

a first implementation of the singular value plot as discussed in #592 #593

forgi86 commented Mar 27, 2021

bnavigator left a comment

bnavigator commented Mar 28, 2021

coveralls commented Mar 28, 2021 •

edited

Loading

bnavigator commented Mar 28, 2021

forgi86 commented Mar 28, 2021

bnavigator commented Mar 28, 2021

bnavigator commented Mar 31, 2021

forgi86 commented Mar 31, 2021

murrayrm Apr 3, 2021

bnavigator Apr 3, 2021

forgi86 Apr 3, 2021

murrayrm Apr 3, 2021

forgi86 Apr 7, 2021

bnavigator Apr 7, 2021

forgi86 Apr 7, 2021

bnavigator commented Apr 11, 2021

forgi86 commented Apr 18, 2021

bnavigator commented Apr 29, 2021

forgi86 commented May 15, 2021

a first implementation of the singular value plot as discussed in #592 #593

a first implementation of the singular value plot as discussed in #592 #593

Conversation

forgi86 commented Mar 27, 2021

bnavigator left a comment

Choose a reason for hiding this comment

bnavigator commented Mar 28, 2021

coveralls commented Mar 28, 2021 • edited Loading

bnavigator commented Mar 28, 2021

forgi86 commented Mar 28, 2021

bnavigator commented Mar 28, 2021

bnavigator commented Mar 31, 2021

forgi86 commented Mar 31, 2021

murrayrm Apr 3, 2021

Choose a reason for hiding this comment

bnavigator Apr 3, 2021

Choose a reason for hiding this comment

forgi86 Apr 3, 2021

Choose a reason for hiding this comment

murrayrm Apr 3, 2021

Choose a reason for hiding this comment

forgi86 Apr 7, 2021

Choose a reason for hiding this comment

bnavigator Apr 7, 2021

Choose a reason for hiding this comment

forgi86 Apr 7, 2021

Choose a reason for hiding this comment

bnavigator commented Apr 11, 2021

forgi86 commented Apr 18, 2021

bnavigator commented Apr 29, 2021

forgi86 commented May 15, 2021

coveralls commented Mar 28, 2021 •

edited

Loading