Add missing labels when returning TimeResponseData by joaoantoniocardoso · Pull Request #892 · python-control/python-control

joaoantoniocardoso · 2023-05-20T16:48:37Z

Fixes #891

murrayrm

Thanks for this nice contribution. I suggest using *_labels instead of *_index, which I think makes the code a bit more readable.

murrayrm · 2023-05-22T02:17:57Z

control/timeresp.py

+        output_labels=sys.output_index, input_labels=sys.input_index,
+        state_labels=sys.state_index,


This is OK, but it might be more clear to use sys.output_labels, etc.

murrayrm · 2023-05-22T02:23:23Z

control/timeresp.py

+    # Select only the given input and output, if any
+    input_index = sys.input_index if input is None else {k: 0 for k, v in sys.input_index.items() if v == input}
+    output_index = sys.output_index if output is None else {k: 0 for k, v in sys.output_index.items() if v == output}
+


Consider changing this to

input_labels = sys.input_labels if input is None \ else sys.input_labels[input] output_labels = sys.output_labels if output is None else \ sys.output_labels[output]

which seems cleaner.

murrayrm · 2023-05-22T02:23:31Z

control/timeresp.py

+        output_labels=output_index, input_labels=input_index,
+        state_labels=sys.state_index,


output_labels=output_labels, input_labels=input_labels, state_labels=sys.state_labels,

murrayrm · 2023-05-22T02:23:46Z

control/timeresp.py

+    # Select only the given output, if any
+    output_index = sys.output_index if output is None else {k: 0 for k, v in sys.output_index.items() if v == output}
+


murrayrm · 2023-05-22T02:23:55Z

control/timeresp.py

+        output_labels=output_index, input_labels=None,
+        state_labels=sys.state_index,


murrayrm · 2023-05-22T02:24:06Z

control/timeresp.py

+    # Select only the given input and output, if any
+    input_index = sys.input_index if input is None else {k: 0 for k, v in sys.input_index.items() if v == input}
+    output_index = sys.output_index if output is None else {k: 0 for k, v in sys.output_index.items() if v == output}
+


murrayrm · 2023-05-22T02:24:16Z

control/timeresp.py

+        output_labels=output_index, input_labels=input_index,
+        state_labels=sys.state_index,


joaoantoniocardoso · 2023-05-22T16:26:20Z

Thanks for this nice contribution. I suggest using *_labels instead of *_index, which I think makes the code a bit more readable.

Thanks, it surely makes it more readable, but I was unsure of using *_labels because the input_output_response is using *_index and I'm not aware of the reasons behind it.. I was suspecting that *_labels might not guarantee the order, which *_index surely does. Are you sure it's okay to use *_labels?

joaoantoniocardoso · 2023-05-26T00:19:17Z

Hi @murrayrm, I did the changes regarding using _labels instead of _index, can you take a look again? Thanks!

control/timeresp.py

joaoantoniocardoso · 2023-05-27T17:20:25Z

control/timeresp.py

+    # Get labels for the inputs/outputs (prior to mimo -> simo conversion)
+    input_labels = sys.input_labels if input is None \
+        else [sys.input_labels[input]]
+    output_labels = sys.output_labels if output is None \
+        else [sys.output_labels[output]]
+


I was thinking about it. Should we move it to here, or should we make the mimo/siso/simo conversions to also propagate the labels (and name)?

Either way works, but because _get_ss_simo is an internal function, it is fine for it to ignore labels.

take a look at this commit

It's your PR, so feel free to revert the code moves that I made and put in your proposed commit. I was just trying to get something that passed the unit tests, since they were failing.

Also: I think it should work to do sys.output_labels[output] instead of [sys.output_labels[output]] (eg, remove the outer list). The code underneath is smart enough to deal with getting passed a string for a single input/output.

Also: I think it should work to do sys.output_labels[output] instead of [sys.output_labels[output]] (eg, remove the outer list). The code underneath is smart enough to deal with getting passed a string for a single input/output.

The _process_labels had a small bug, now it works with plain str instead of [str]

…el them

joaoantoniocardoso · 2023-05-27T18:46:32Z

@murrayrm I'm good with the patch, let me know if you need any changes, otherwise feel free to merge it

joaoantoniocardoso force-pushed the patch-2 branch from a75a962 to 0a87017 Compare May 20, 2023 19:37

murrayrm reviewed May 22, 2023

View reviewed changes

joaoantoniocardoso force-pushed the patch-2 branch 6 times, most recently from 7b8db61 to 4ed8491 Compare May 26, 2023 00:03

joaoantoniocardoso requested a review from murrayrm May 26, 2023 00:19

joaoantoniocardoso commented May 26, 2023

View reviewed changes

control/timeresp.py Outdated Show resolved Hide resolved

joaoantoniocardoso force-pushed the patch-2 branch from 4ed8491 to 7daed4b Compare May 27, 2023 01:21

joaoantoniocardoso commented May 27, 2023

View reviewed changes

joaoantoniocardoso and others added 5 commits May 27, 2023 15:40

Add missing labels when returning TimeResponseData

34a7a49

Test if labels are transferred to the response and we can still relab…

a78e85c

…el them

use *_labels in input_output_response for consistency

56c72c7

Propagate labels and names when converting between mimo/siso/simo

d744a54

Fix _process_labels to accept a single str label

bd83e8c

joaoantoniocardoso force-pushed the patch-2 branch from 8d0fb10 to bd83e8c Compare May 27, 2023 18:40

murrayrm merged commit 147d531 into python-control:main May 27, 2023

murrayrm added this to the 0.9.4 milestone Jun 7, 2023

		output_labels=sys.output_index, input_labels=sys.input_index,
		state_labels=sys.state_index,

		output_labels=output_index, input_labels=input_index,
		state_labels=sys.state_index,

		# Select only the given output, if any
		output_index = sys.output_index if output is None else {k: 0 for k, v in sys.output_index.items() if v == output}

Comments

Conversation

joaoantoniocardoso commented May 20, 2023

Uh oh!

murrayrm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joaoantoniocardoso commented May 22, 2023

Uh oh!

joaoantoniocardoso commented May 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

murrayrm May 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joaoantoniocardoso commented May 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

joaoantoniocardoso commented May 26, 2023 •

edited

Loading

murrayrm May 27, 2023 •

edited

Loading