Input-output selection in `bode_plot` #1916

MohamedAdelNaguib · 2023-01-10T23:41:27Z

Problem statement :
Currently, the bode_plot function plots multiple graphs/plots depending on the values of dim_input and dim_output.

Goal:
The goal is to restrict one or more input-output pair from these plots to be displayed to the user instead of displaying all the plots.

Solution:
By introducing two new optional arguments , input_indices and output_indices, which indicate the index of the input or the output that we are interested in.

Examples:
If we have dim_input=3, dim_output=3,

By passing output_indices = [1,2] and input indices = [0,2] that will result in displaying all the subplots with the following indices {(1,0), (1,2), (2,0), (2,2)}. also if output_indices=None and input_indices = [1,2] the function should display the subplot with the following indices {(0,1) , (0,2) , (1,1), (1,2) , (2,1) ,(2,2)}.
By passing input_indices=None and output_indices=[1,2], that will result in displaying the subplots with the following indices {(1,0),(1,1),(1,2),(2,0),(2,1),(2,2)}.
if you did not specify a value for both input_indices and output_indices, all the subplots will be dispalyed

I tested my implementation by using the same code here #1615 with some edits in the bode_plot calls giving it different values for the new added parameters input_indices and output_indices and it gives me the corresponding plots of my chosen indices.

closes #1767

pmli

Thanks for the PR. Below are two comments.

It would be good to also add a test or edit the LTI system tutorial.

src/pymor/models/transfer_function.py

pmli

Looks much better now, I just have a few minor remarks below (the point about adding a test or editing the tutorial remains).

src/pymor/models/transfer_function.py

MohamedAdelNaguib · 2023-01-12T15:29:36Z

@pmli, For adding a tutorial part I am currently still working on it. sorry for being late but I think I found a bug so I am further investigating it before opening a new issue for it.

pmli

A few minor comments. I'll take a closer look tomorrow.

src/pymor/models/transfer_function.py

docs/source/tutorial_lti_systems.md

pmli · 2023-01-14T17:09:45Z

Oh, I just remembered, could you add yourself to AUTHORS.md @MohamedAdelNaguib? You would just need to start a new "pyMOR 2023.1" section under "Contributors".

HenKlei

Hi @MohamedAdelNaguib, thanks for your contribution! I have some minor comments, mostly regarding documentation.

@pmli, @MohamedAdelNaguib, what do you think about adding a subtitle to the plots (respectively the rows and columns) to make clear which inputs and outputs are displayed in the respective plot?

src/pymor/models/transfer_function.py

artpelling

Hi @MohamedAdelNaguib, welcome to pyMOR ✨

Thanks for your contribution, this feature is long overdue. I left some comments, let me know what you think.

@pmli This seems also related to the slicing of LTIModels we talked about. At the moment, restricting the indices wouldn't really result in faster computations. If we had a slicing mechanism, we could envoke it here.
However, I'm not sure when we actually get around to implement that..

src/pymor/models/transfer_function.py

HenKlei

Apart from the wording, this now looks good to me.

What do you think @artpelling, @pmli?

src/pymor/models/transfer_function.py

HenKlei · 2023-01-16T15:10:28Z

You should rebase on main @MohamedAdelNaguib, since AUTHORS.md has changed.

src/pymor/models/transfer_function.py

artpelling

Apart from the import sorting, this looks good to me now 👍

src/pymor/models/transfer_function.py

pmli · 2023-01-16T22:34:25Z

@MohamedAdelNaguib Looking at the commits, it seems like some other PRs got mixed in. Could you try rebasing again on the main branch? On my system, I could do it by running git rebase -i main, deleting commits that start with a bracket "[", fixing the conflict in AUTHORS.md twice, and running git rebase --continue once more.

@pmli, @MohamedAdelNaguib, what do you think about adding a subtitle to the plots (respectively the rows and columns) to make clear which inputs and outputs are displayed in the respective plot?

I thought about this, and I think it would be nice to do something similar to Matlab, but I don't know how to nicely add both horizontal and vertical subtitles. That's way I thought it would be better to do it later in another PR.

@pmli This seems also related to the slicing of LTIModels we talked about. At the moment, restricting the indices wouldn't really result in faster computations. If we had a slicing mechanism, we could envoke it here. However, I'm not sure when we actually get around to implement that..

Yes, I agree it would be good to have slicing here, and that we'll do it some other time.

MohamedAdelNaguib · 2023-01-17T00:23:05Z

@MohamedAdelNaguib Looking at the commits, it seems like some other PRs got mixed in. Could you try rebasing again on the main branch? On my system, I could do it by running git rebase -i main, deleting commits that start with a bracket "[", fixing the conflict in AUTHORS.md twice, and running git rebase --continue once more.

@pmli I am currently working on it but there are a lot of issues while rebase so I am trying to solve them.

pmli · 2023-01-19T14:08:06Z

Ok, just let me know if you get stuck.

…ion of a specific input-output

…ion<3.10

…ew comments

…nd the axs isn't None

…the new arguments

…ion of a specific input-output

pmli · 2023-01-20T18:23:46Z

Thanks @MohamedAdelNaguib and congrats for your first pyMOR contribution 🎉
Thanks @HenKlei and @artpelling for the review.

MohamedAdelNaguib · 2023-01-21T02:09:42Z

Thank @pmli, @HenKlei and @artpelling for your help and support during this process.

github-actions bot added the autoupdate label Jan 10, 2023

MohamedAdelNaguib marked this pull request as draft January 11, 2023 09:24

MohamedAdelNaguib marked this pull request as ready for review January 11, 2023 09:24

MohamedAdelNaguib marked this pull request as draft January 11, 2023 09:24

MohamedAdelNaguib marked this pull request as ready for review January 11, 2023 09:28

pmli self-assigned this Jan 11, 2023

pmli added this to the 2023.1 milestone Jan 11, 2023

pmli added the pr:new-feature Introduces a new feature label Jan 11, 2023

MohamedAdelNaguib changed the title ~~Input-output selection in bode_plot #1767~~ Input-output selection in bode_plot Jan 11, 2023

pmli requested changes Jan 11, 2023

View reviewed changes

src/pymor/models/transfer_function.py Outdated Show resolved Hide resolved

src/pymor/models/transfer_function.py Outdated Show resolved Hide resolved

MohamedAdelNaguib marked this pull request as draft January 11, 2023 21:32

pmli requested changes Jan 12, 2023

View reviewed changes

MohamedAdelNaguib marked this pull request as ready for review January 12, 2023 17:28

MohamedAdelNaguib requested a review from pmli January 12, 2023 22:06

pmli requested changes Jan 12, 2023

View reviewed changes

src/pymor/models/transfer_function.py Outdated Show resolved Hide resolved

docs/source/tutorial_lti_systems.md Outdated Show resolved Hide resolved

MohamedAdelNaguib requested a review from pmli January 13, 2023 23:23

pmli requested changes Jan 14, 2023

View reviewed changes

docs/source/tutorial_lti_systems.md Outdated Show resolved Hide resolved

docs/source/tutorial_lti_systems.md Outdated Show resolved Hide resolved

pmli approved these changes Jan 14, 2023

View reviewed changes

pmli requested a review from artpelling January 14, 2023 14:53

HenKlei reviewed Jan 16, 2023

View reviewed changes

artpelling reviewed Jan 16, 2023

View reviewed changes

src/pymor/models/transfer_function.py Outdated Show resolved Hide resolved

src/pymor/models/transfer_function.py Outdated Show resolved Hide resolved

MohamedAdelNaguib requested review from artpelling and HenKlei and removed request for artpelling January 16, 2023 14:57

HenKlei approved these changes Jan 16, 2023

View reviewed changes

src/pymor/models/transfer_function.py Outdated Show resolved Hide resolved

src/pymor/models/transfer_function.py Outdated Show resolved Hide resolved

artpelling requested changes Jan 16, 2023

View reviewed changes

src/pymor/models/transfer_function.py Outdated Show resolved Hide resolved

artpelling self-requested a review January 16, 2023 18:04

artpelling approved these changes Jan 16, 2023

View reviewed changes

src/pymor/models/transfer_function.py Outdated Show resolved Hide resolved

MohamedAdelNaguib added 17 commits January 19, 2023 16:11

Adding input_indices and output_inices to bode_plot to enable seleect…

1eae87a

…ion of a specific input-output

Replace match-case with if-else statement as it is not supported vers…

70ca12a

…ion<3.10

Refcatorizaing the code to remove redundancies according to the rewvi…

fbe6867

…ew comments

Change varible namings along with some code refactorizing

f512a91

Fixing an assertion error happen when the number of inout equals to a…

f75c4fb

…nd the axs isn't None

Extending the LTI tutorial with another bode_plot function call with …

21d7aad

…the new arguments

Fixing a typo in the LTI tutorial

d711305

Typo changes and remove printing statement

09e64f6

Adding spaces to keep the consestency of the code

ef2969e

Changes to the LTI tutorial as mentioned in the review

1e1bf9f

Adding my username in the authors

a1714a8

refactorizing the code by removing the _cal_plot_dimentsion

1ec8743

Adding input_indices and output_inices to bode_plot to enable seleect…

8d025dc

…ion of a specific input-output

Adding my username in the authors

6dfdafe

refactorizing the code by removing the _cal_plot_dimentsion

0b7d55d

Follow the review instructions

dbb7809

sorting import statement alphabetically

2354eb6

MohamedAdelNaguib force-pushed the input-output_selection_bode_plot branch from c913d6b to 2354eb6 Compare January 19, 2023 15:15

pmli merged commit 6532dbf into pymor:main Jan 20, 2023

MohamedAdelNaguib deleted the input-output_selection_bode_plot branch January 22, 2023 17:10

artpelling mentioned this pull request Feb 13, 2023

IO Slicing of LTIModel #1955

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Input-output selection in `bode_plot` #1916

Input-output selection in `bode_plot` #1916

MohamedAdelNaguib commented Jan 10, 2023 •

edited

pmli left a comment

pmli left a comment

MohamedAdelNaguib commented Jan 12, 2023

pmli left a comment

pmli commented Jan 14, 2023

HenKlei left a comment

artpelling left a comment

HenKlei left a comment

HenKlei commented Jan 16, 2023

artpelling left a comment

pmli commented Jan 16, 2023

MohamedAdelNaguib commented Jan 17, 2023 •

edited by pmli

pmli commented Jan 19, 2023

pmli commented Jan 20, 2023

MohamedAdelNaguib commented Jan 21, 2023

Input-output selection in bode_plot #1916

Input-output selection in bode_plot #1916

Conversation

MohamedAdelNaguib commented Jan 10, 2023 • edited

pmli left a comment

Choose a reason for hiding this comment

pmli left a comment

Choose a reason for hiding this comment

MohamedAdelNaguib commented Jan 12, 2023

pmli left a comment

Choose a reason for hiding this comment

pmli commented Jan 14, 2023

HenKlei left a comment

Choose a reason for hiding this comment

artpelling left a comment

Choose a reason for hiding this comment

HenKlei left a comment

Choose a reason for hiding this comment

HenKlei commented Jan 16, 2023

artpelling left a comment

Choose a reason for hiding this comment

pmli commented Jan 16, 2023

MohamedAdelNaguib commented Jan 17, 2023 • edited by pmli

pmli commented Jan 19, 2023

pmli commented Jan 20, 2023

MohamedAdelNaguib commented Jan 21, 2023

Input-output selection in `bode_plot` #1916

Input-output selection in `bode_plot` #1916

MohamedAdelNaguib commented Jan 10, 2023 •

edited

MohamedAdelNaguib commented Jan 17, 2023 •

edited by pmli