Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Bode plot #1051

Merged
merged 11 commits into from
Aug 26, 2020
Merged

Add Bode plot #1051

merged 11 commits into from
Aug 26, 2020

Conversation

pmli
Copy link
Member

@pmli pmli commented Aug 7, 2020

This PR adds the bode_plot method to InputOutputModel for drawing the Bode plot (with both magnitude and phases).

Also, an example is added to the BT tutorial. I used the constrained_layout option since the suptitle is otherwise too high, but it is an experimental feature. The documentation suggests getting axes positions and setting them explicitly for reproducibility, but I guess this would have to be done for all 12 subplots...

See #389.

Update: This PR also adds the bode method that returns magnitudes and phases used in bode_plot.

@pmli pmli added the pr:new-feature Introduces a new feature label Aug 7, 2020
@pmli pmli added this to the 2020.2 milestone Aug 7, 2020
@codecov
Copy link

codecov bot commented Aug 7, 2020

Codecov Report

Merging #1051 into master will decrease coverage by 0.16%.
The diff coverage is 7.69%.

Impacted Files Coverage Δ
src/pymor/models/iosys.py 45.36% <7.69%> (-2.14%) ⬇️
src/pymor/vectorarrays/list.py 84.15% <0.00%> (-0.19%) ⬇️
src/pymor/vectorarrays/numpy.py 84.55% <0.00%> (+0.24%) ⬆️

@pmli pmli requested review from lbalicki and sdrave August 21, 2020 10:16
@lbalicki
Copy link
Member

Overall the Bode plot seems to work well. Maybe we should change the default options of the plot a little bit, such that it looks better for more examples?

With the default options of the current implementation, the example fom from the BT tutorial and w = np.logspace(10, -10, 100) in a Jupyter notebook I get the plot:
image

I would suggest initializing the fig, ax objects via fig, ax = plt.subplots(2 * self.output_dim, self.input_dim, sharex=True, figsize=(3 * self.input_dim, 4 * self.output_dim), constrained_layout=True), which for me results in "nice" looking graphs for a lot of different input and output dimensions. What do you think?

Comment on lines 112 to 140
fig = plt.gcf()
ax = fig.subplots(2 * self.output_dim, self.input_dim, sharex=True, squeeze=False)
Copy link
Member

Choose a reason for hiding this comment

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

Change to fig, ax = plt.subplots(2 * self.output_dim, self.input_dim, sharex=True, figsize=(3 * self.input_dim, 4 * self.output_dim), constrained_layout=True)?

@pmli
Copy link
Member Author

pmli commented Aug 24, 2020

I would suggest initializing the fig, ax objects via fig, ax = plt.subplots(2 * self.output_dim, self.input_dim, sharex=True, figsize=(3 * self.input_dim, 4 * self.output_dim), constrained_layout=True), which for me results in "nice" looking graphs for a lot of different input and output dimensions. What do you think?

Sounds good. Maybe it would be good to also use plt.rcParams['figure.figsize']. I'll try and see what works.

@pmli
Copy link
Member Author

pmli commented Aug 25, 2020

Now the default Bode plot size is [self.input_dim * width, 2 * self.output_dim * height], where width, height = plt.rcParams['figure.figsize'] and constrained_layout is used. It seems that constrained_layout makes the subplots bigger, probably because it fills the margins. I'm not sure how to fix this besides just scaling the width and height.

Additionally, I found that np.unwrap can make a sequence of phases more continuous and moved the computation of magnitudes and phases to a separate bode method (similar to scipy.signal.bode).

@pmli pmli merged commit 9ddddfc into master Aug 26, 2020
@pmli pmli deleted the bode_plot branch August 26, 2020 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:new-feature Introduces a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants