GeezerVison

Coverage decreased (-0.2%) to 84.044% when pulling b2c8d44 on samlaf:interconnectedsystems into d3142ff on python-control:master.

Here's a short code snippet to visualize the signal names for different Connections:

import control

nlios111 = control.NonlinearIOSystem(
    lambda t,x,u,params: x, inputs=2, outputs=2, states=2
)
nlios101 = control.NonlinearIOSystem(
    None, lambda t,x,u,params: u, inputs=2, outputs=2
)

series = nlios111 * nlios101
parallel = nlios111 + nlios101
neg = - nlios111
feedback = nlios101.feedback(nlios111)
dup = nlios111 * nlios111.copy() #if we remove the .copy() we get a warning and names are messed up
hierarchical = series*nlios111

print("1 input, 1 state, 1 output:\n", nlios111, "\n")
print("1 input, no state, 1 output:\n", nlios101, "\n")
print("Series connection: sys[0] * sys[1]\n", series, "\n")
print("Parallel connection: sys[0] + sys[1]\n", parallel, "\n")
print("Negative connection: -sys[0]\n", neg, "\n")
print("Feedback connection: sys[1].feedback(sys[0])\n", feedback, "\n")
print("Connection with duplicate systems: sys[0] * sys[0].copy()\n", dup, "\n")
print("Hierarchical connection: series * sys[0]\n", hierarchical)

And the output after my last push:

1 input, 1 state, 1 output:
System: sys[0]
Inputs (2): u[0], u[1],
Outputs (2): y[0], y[1],
States (2): x[0], x[1],

1 input, no state, 1 output:
System: sys[1]
Inputs (2): u[0], u[1],
Outputs (2): y[0], y[1],
States (0):

Series connection: sys[0] * sys[1]
System: sys[2]
Inputs (2): u[0], u[1],
Outputs (2): y[0], y[1],
States (2): sys[0].x[0], sys[0].x[1],

Parallel connection: sys[0] + sys[1]
System: sys[3]
Inputs (2): u[0], u[1],
Outputs (2): y[0], y[1],
States (2): sys[0].x[0], sys[0].x[1],

Negative connection: -sys[0]
System: sys[4]
Inputs (2): u[0], u[1],
Outputs (2): y[0], y[1],
States (2): sys[0].x[0], sys[0].x[1],

Feedback connection: sys[1].feedback(sys[0])
System: sys[5]
Inputs (2): u[0], u[1],
Outputs (2): y[0], y[1],
States (2): sys[0].x[0], sys[0].x[1],

Connection with duplicate systems: sys[0] * sys[0].copy()
System: sys[7]
Inputs (2): u[0], u[1],
Outputs (2): y[0], y[1],
States (4): sys[6].x[0], sys[6].x[1], sys[0].x[0], sys[0].x[1],

Hierarchical connection: series * sys[0]
System: sys[8]
Inputs (2): u[0], u[1],
Outputs (2): y[0], y[1],
States (4): sys[0].x[0], sys[0].x[1], sys[2].sys[0].x[0], sys[2].sys[0].x[1],

@samlaf Can you add some unit tests to cover your new code and show that things are working? The examples that you have in your comment from 20 May would probably be fine. You might also try mixing named systems and named signals with unnamed systems/signals to make sure that nothing funny happens. And, as a harder test, try creating a system/signal with a name that would conflict with the default name (eg, create a system whose name is sys[1] and combine it with an unnamed systems).

@samlaf Can you add some unit tests to cover your new code and show that things are working? The examples that you have in your comment from 20 May would probably be fine. You might also try mixing named systems and named signals with unnamed systems/signals to make sure that nothing funny happens. And, as a harder test, try creating a system/signal with a name that would conflict with the default name (eg, create a system whose name is sys[1] and combine it with an unnamed systems).

Should I be enforcing sys/signals names here? Wouldn't this render the API a bit too rigid (in case people want to change the naming conventions later, etc.)
Otherwise, what kind of unit tests did you have in mind?

Unit tests are failing because

nlsys * nlsys

no longer works. It seems like it should be possible to take two copies of a system and put them in series (which is what this does) without generating an error.

Thank you for pointing this out @murrayrm, and thanks a lot to both of you two for your patience... newbie at work here.
I was concentrating on the signal_names test for too long and forget to rerun the other tests afterwards. Should be fixed now.

The system naming convention code should be fine now. Still need to write the signal name convention unit test though.

Looks like git got confused by the new unit test. I don't see any actual conflicts, but you may need to fiddle with things to allow the automatic merge check to work.

It seems like a conflict between my branch and master branch?

That's what it's saying, but what's odd is that I don't see the conflict. You added in a test and it shouldn't conflict with other tests. If you agree, I can try to fix it via the web interface and we'll see if that gets things running again.

Sure, please do. I wouldn't know what to try anyways and would be scared to break other things.

I added the last unit tests. This is ready to be reviewed and hopefully should be close to done.

The changes remove test coverage of the functions set_input_map() and set_output_map()
https://coveralls.io/builds/32204258/source?filename=control%2Fiosys.py#L1284

In fact, those functions are not used by the whole package anymore. But they are public API. So a unit test for them should be present. (I would personally prefer that they are created in the style of #438. Feel free to add them as a PR against the bnavigator:array-matrix-tests branch)

Sure I can do that.
Just to make sure I understand, I think these functions were used in the add, mul, etc. functions which interconnect systems. Before they would create a blank interconnected system and then use the setters like set_input_map to populate the object. I changed it so that the "setting" was done in the init code.
Anyways, my question is: this code being "covered" before means that the unit test was calling them? So even if there wasn't a designed unit test for these specific functions, at least if they had caused a bug that would have caused some unit test to fail, but now that's not the case anymore, so we should add an explicit unit test. Is this correct?

Yes, that is correct. The unit tests should make sure that using those functions results in the documented behavior. Previously, this was more or less implicit by the tests when __add__ , __mul__. __neg__ and feedback were called.

Hi @samlaf, if you still intend to update your PR with the requested unit tests, please do so based on the iosys_test.py in #438. We are discussing in #451 about the proper merge sequence, and #438 will likely be the first.

Hi @bnavigator, I was planning on doing these, but I got major neck pain recently, and am spending some time away from the keyboard. Are you waiting on me to merge the other PRs? I'm not sure when I'll have time to write these, but if it's not a hurry I will eventually get to them.

No hurry, get well soon! As I wrote, I removed the dependency in my PR so your PR can be merged later. Whenever you can get to it. You will have to rebase.