Add missing labels when returning TimeResponseData #892

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing labels when returning TimeResponseData #892

Add missing labels when returning TimeResponseData #892

joaoantoniocardoso commented May 20, 2023

murrayrm left a comment

murrayrm May 22, 2023

murrayrm May 22, 2023

murrayrm May 22, 2023

murrayrm May 22, 2023

murrayrm May 22, 2023

murrayrm May 22, 2023

murrayrm May 22, 2023

joaoantoniocardoso commented May 22, 2023

joaoantoniocardoso commented May 26, 2023 •

edited

Loading

joaoantoniocardoso May 27, 2023

murrayrm May 27, 2023

joaoantoniocardoso May 27, 2023

murrayrm May 27, 2023 •

edited

Loading

murrayrm May 27, 2023

joaoantoniocardoso May 27, 2023

joaoantoniocardoso commented May 27, 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}

Add missing labels when returning TimeResponseData #892

Add missing labels when returning TimeResponseData #892

Conversation

joaoantoniocardoso commented May 20, 2023

murrayrm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joaoantoniocardoso commented May 22, 2023

joaoantoniocardoso commented May 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

murrayrm May 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joaoantoniocardoso commented May 27, 2023

joaoantoniocardoso commented May 26, 2023 •

edited

Loading

murrayrm May 27, 2023 •

edited

Loading