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

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

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

Conversation

forgi86
Copy link
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)

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

@bnavigatorbnavigator left a 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

@bnavigator
Copy link
Contributor

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

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

coveralls commentedMar 28, 2021•
edited
Loading

Coverage Status

Coverage increased (+0.1%) to 89.602% when pulling649c394 on forgi86:singular-values-plot intoa6a5bee on python-control:master.

forgi86and others added3 commitsMarch 28, 2021 13:05
Co-authored-by: Ben Greiner <code@bnavigator.de>
- removed deprecated handling 'Plot'- fixed docstring for argument plot the singular value plot
@bnavigator
Copy link
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

usesqueeze=False

- added default settings for singular_values_plot
@forgi86
Copy link
ContributorAuthor

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

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

- use omega_sys to compute omega_complex!
@forgi86
Copy link
ContributorAuthor

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
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
Copy link
Member

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 equivalentbode configuration variable. Is someone going to use one set of defaults for Bode and a separate one for singular values? (Also note thatgang_of_four uses the Bode plot defaults.)

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

bnavigator reacted with thumbs up emoji
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
ContributorAuthor

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?

Copy link
Member

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 theuse_matlab_defaults anduse_fbs_defaults functions inconfig.py. I'm not sure about what to do withbode.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 usefreqplot for everything except that. I would probably just usefreqplot for everything.

sawyerbfuller reacted with thumbs up emoji
Copy link
ContributorAuthor

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

Copy link
Contributor

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 ofset_defaults and_get_param in an elegant way.

Copy link
ContributorAuthor

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.

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

@forgi86
Copy link
ContributorAuthor

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

@sawyerbfullersawyerbfuller linked an issueApr 20, 2021 that may beclosed by this pull request
@bnavigator
Copy link
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.

@bnavigatorbnavigator merged commitdb174b7 intopython-control:masterApr 29, 2021
@forgi86
Copy link
ContributorAuthor

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

@murrayrmmurrayrm added this to the0.9.1 milestoneDec 30, 2021
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@murrayrmmurrayrmmurrayrm left review comments

@bnavigatorbnavigatorbnavigator requested changes

@sawyerbfullersawyerbfullersawyerbfuller approved these changes

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