Skip to content

NonlinearIOSystem.__str__() should not fail for successfully constructed systems #329

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.

Sign up for GitHub

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

Jump to bottom
Closed
rabraker opened this issue Jul 21, 2019 · 2 comments
Closed

NonlinearIOSystem.__str__() should not fail for successfully constructed systems #329

rabraker opened this issue Jul 21, 2019 · 2 comments
Assignees
@murrayrm
Labels
bug
Milestone
0.8.3

Comments

@rabraker
Copy link
Contributor

The __str__ method in InputOutputSystem assumes that the ninputs, noutputs and nstates have all been set to integers, but the constructors do not guarantee this. For example:

>> sys = ctl.NonlinearIOSystem(lambda t, x, u, params: x)
>> print(sys)

---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-4-85c64df40808> in <module>()
----> 1 print(sys)

/home/arnold/pythonBox/control_dev/python-control-rabraker/control/iosys.py in __str__(self)
    214         """String representation of an input/output system"""
    215         str = "System: " + (self.name if self.name else "(none)") + "\n"
--> 216         str += "Inputs (%d): " % self.ninputs
    217         for key in self.input_index: str += key + ", "
    218         str += "\nOutputs (%d): " % self.noutputs

TypeError: %d format: a number is required, not NoneType

I would think that once a system is constructed, __str__() should always succeed. The simplest way to fix this is to replace %d with %s in the format string, so it can accept NoneType. I can submit a PR and add a regression test if this is the desired fix.

This behavior also means that when noutputs etc are not set, many of the overload operators give unhelpful errors

>>> sys = ctl.NonlinearIOSystem(lambda t, x, u, params: x)
>>> sys2 = sys * sys
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-27-ea1505c3dfb9> in <module>()
----> 1 sys2 = sys * sys

/home/arnold/pythonBox/control_dev/python-control-rabraker/control/iosys.py in __mul__(sys2, sys1)
    252 
    253         # Return the series interconnection between the systems
--> 254         newsys = InterconnectedSystem((sys1, sys2))
    255 
    256         #  Set up the connecton map

/home/arnold/pythonBox/control_dev/python-control-rabraker/control/iosys.py in __init__(self, syslist, connections, inplist, outlist, inputs, outputs, states, params, dt, name)
    949                 raise TypeError("System '%s' must define number of inputs, "
    950                                 "outputs, states in order to be connected" %
--> 951                                 sys)
    952 
    953             # Keep track of the offsets into the states, inputs, outputs

/home/arnold/pythonBox/control_dev/python-control-rabraker/control/iosys.py in __str__(self)
    214         """String representation of an input/output system"""
    215         str = "System: " + (self.name if self.name else "(none)") + "\n"
--> 216         str += "Inputs (%d): " % self.ninputs
    217         for key in self.input_index: str += key + ", "
    218         str += "\nOutputs (%d): " % self.noutputs

TypeError: %d format: a number is required, not NoneType

As something of a side note/question/point for discussion: I'm having a hard time groking why ninputs, noutputs and nstates are allowed to be None. Is there a downside to making inputs etc a required rather than optional argument in the NonlinearIOSystem constructor? It seems that might make the class easier to use because then all the operators (__mul__() etc) would always work and not depend on how the user called the constructor.
@murrayrm
Copy link
Member

Completely agree that these should not be failing. Will work on a fix.

The original reason for allowing ninputs, noutputs, nstates to be None was to allow users to just pass the functions defining an input/output system and then have the various member functions "figure out" the dimensions when needed. So, for example, if you linearize a system about an operating point x0, u0 then you can tell from the dimensions of x0 and u0 how many states and inputs you have. Then when you call the output function, you can figure out how many outputs you have.

Not sure if that functionality is going to be that useful, but at the least there should not be errors of the sort you are pointing out above.

@murrayrm murrayrm self-assigned this Jul 22, 2019
@murrayrm murrayrm added the bug label Jul 22, 2019
@murrayrm murrayrm mentioned this issue Aug 2, 2019
Closed
@murrayrm murrayrm added this to the 0.8.3 milestone Aug 9, 2019
@murrayrm murrayrm mentioned this issue Dec 30, 2019
Merged
@murrayrm
Copy link
Member

Fixed in PR #355

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@murrayrm murrayrm

Labels
bug
Projects
None yet
Milestone
0.8.3
Development

No branches or pull requests
2 participants
@murrayrm @rabraker