Cleanup TransferFunction Input Sanitization #135

dapperfu · 2017-02-18T19:02:51Z

This started out to address #112 & #130, but I found a few other small edge cases.

To simplify unit testing I broke the input cleaning out to a separate function and added as many test cases as I could think of.

Deep copy

It performs a deep copy on args before anything else.

Test Code:

num = [[[1], [2]]]
den = [[[1,1], [2,1]]]
print(num)
print(den)
control.TransferFunction(num, den)
print(num)
print(den)

Before:

[[[1], [2]]]
[[[1, 1], [2, 1]]]
[[array([1]), array([2])]]
[[array([1, 1]), array([2, 1])]]

After:

[[[1], [2]]]
[[[1, 1], [2, 1]]]
[[[1], [2]]]
[[[1, 1], [2, 1]]]

Validate All Input Types:

The previous input validation would only check the first element if it was a valid input type. This means you could feed in extra bad values in other positions and it would accept it, only to fail later.

Test Code:

num = [[[1,"a"]]]
den = [[[2,1]]]
print(num)
print(den)
sys = control.TransferFunction(num, den)
print(num)
print(den)

Before:

It would accept the values (and mutate the inputs):

[[[1, 'a']]]
[[[2, 1]]]
[[array(['1', 'a'], 
      dtype='<U21')]]
[[array([2, 1])]]

But fail when you tried to access the system:

   1013 
   1014     for k in range(len(coeffs)):
-> 1015         coefstr = '%.4g' % abs(coeffs[k])
   1016         if coefstr[-4:] == '0000':
   1017             coefstr = coefstr[:-5]

TypeError: bad operand type for abs(): 'numpy.str_'

After

It fails immediately:

TypeError: The numerator and denominator inputs must be scalars or vectors (for
SISO), or lists of lists of vectors (for SISO or MIMO).

Added more valid types.

If you passed an numpy array as an input it would possibly fail depending on which architecture you were using it on. int resolves to int64 on 64-bit systems. It now accepts valid types of int, int8, int16, int32, int64, float, float16, float32, float64, float128. (If for some reason your transfer function parts were generated in another function).

Code:

import control
from numpy import array, int
num = array(1, dtype=int)
den = array([2, 1], dtype=int)
control.TransferFunction(num, den)

Before:

TypeError: The numerator and denominator inputs must be scalars or vectors (for
SISO), or lists of lists of vectors (for SISO or MIMO).

After:

    1
-------
2 s + 1

Added Tuple as valid collection for MIMO systems

It was easy enough to add and a valid ordered Python collection.

Given:

# Input 1 to Output 1
num11=[1]
den11=[2, 3]
# Input 1 to Output 2
num12=[4]
den12=[5, 6]
# Input 2 to Output 1
num21=[7]
den21=[8, 9]
# Input 2 to Output 2
num22=[10]
den22=[11, 12]

Previously this was the only valid MIMO input:

control.TransferFunction([[num11, num21],[num12, num22]], [[den11, den21],[den12, den22]])

Now all of these are also valid:

control.TransferFunction([(num11, num21),(num12, num22)], [(den11, den21),(den12, den22)])
control.TransferFunction(((num11, num21),(num12, num22)), ((den11, den21),(den12, den22)))
control.TransferFunction(([num11, num21],[num12, num22]), ([den11, den21],[den12, den22]))

Added IO cleanup function to xferfcn module.

From .eggs/README.txt: "This directory contains eggs that were downloaded by setuptools to build, test, and run plug-ins. This directory caches those eggs to prevent repeated downloads. However, it is safe to delete this directory."

…function. Added unit tests to test the cleanup function. Added deep copy to TransferFunction args.

coveralls · 2017-02-18T19:06:01Z

Coverage increased (+0.08%) to 77.408% when pulling 509ff8e on jed-frey:InputSanitization into 5ab74cf on python-control:master.

murrayrm · 2017-02-18T19:06:05Z

control/xferfcn.py

@@ -1349,3 +1317,42 @@ def tfdata(sys):
    tf = _convertToTransferFunction(sys)

    return (tf.num, tf.den)
+
+def cleanPart(data):


Since this is an internal function, should we name it _cleanPart?

Makes sense to me. I just made the changes and pushed.

dapperfu · 2017-02-18T19:10:20Z

*Created a branch and then merged the branch so that I can continue working. I wasn't aware that GitHub pull requests always tracked master, even after the request was created.

@murrayrm

…suggested by @murrayrm

coveralls · 2017-02-18T19:14:52Z

Coverage increased (+0.07%) to 77.4% when pulling d44489f on jed-frey:InputSanitization into 5ab74cf on python-control:master.

roryyorke

Looks good! Comments are all nitpicks.

roryyorke · 2017-03-17T19:03:47Z

control/tests/xferfcn_input_test.py

+#!/usr/bin/env python
+#
+# xferfcn_test.py - test TransferFunction class
+# RMM, 30 Mar 2011 (based on TestXferFcn from v0.4a)


Probably worthwhile correcting this header comment

roryyorke · 2017-03-17T19:06:28Z

control/tests/xferfcn_input_test.py

+        np.testing.assert_array_equal(num_[0][1], array([2.0, 2.0], dtype=float))
+
+    def testListListListFloats(self):
+        """Test 2 lists of ints in a list in a list."""


floats, not ints?

roryyorke · 2017-03-17T19:15:39Z

control/xferfcn.py

@@ -10,6 +10,7 @@
 # Python 3 compatibility (needs to go here)
 from __future__ import print_function
 from __future__ import division
+from __future__ import absolute_import


Had to look this up. It appears to be necessary for Python 2.5 - do we support that? I didn't quite understand what the related PEP was getting at, but it's not obvious to me that your changes especially need the features it enables anyway.

True, I just used my boilerplate "I need to make something Python 2/3 compatible" code. It can probably be removed.

roryyorke · 2017-03-17T19:17:35Z

control/xferfcn.py

+    data: numerator or denominator of a transfer function.
+
+    Returns:
+    data: correctly formatted transfer function part.


you could be a little more specific: return data as a list of lists of ndarrays of type float.

roryyorke · 2017-03-17T19:45:21Z

control/xferfcn.py

+                   float, float16, float32, float64, float128)
+    valid_collection = (list, tuple, ndarray)
+
+    if (isinstance(data, valid_types) or


I was going to suggest using np.isreal instead of isinstance(..,valid_types), but then I found np.isreal(file('/dev/null')) returns True (oops!).

roryyorke · 2017-06-24T08:41:03Z

FWIW, this fixes another bug:

(ctrl-py35) rory@rory-Inspiron-3521:~/Documents/python-control$ (cd ~/src/python-control && git checkout master)
Switched to branch 'master'
Your branch is up-to-date with 'upstream/master'.
(ctrl-py35) rory@rory-Inspiron-3521:~/Documents/python-control$ python -c "from control import tf; g=tf([[[1]]],[[[1,1]]]); print(g+g)"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/rory/src/python-control/control/xferfcn.py", line 349, in __add__
    return TransferFunction(num, den, dt)
  File "/home/rory/src/python-control/control/xferfcn.py", line 163, in __init__
    MIMO).")
TypeError: The numerator and denominator inputs must be scalars or vectors (for
SISO), or lists of lists of vectors (for SISO or MIMO).
(ctrl-py35) rory@rory-Inspiron-3521:~/Documents/python-control$ (cd ~/src/python-control && git checkout InputSanitization)
Switched to branch 'InputSanitization'
Your branch is up-to-date with 'jed-frey/InputSanitization'.
(ctrl-py35) rory@rory-Inspiron-3521:~/Documents/python-control$ python -c "from control import tf; g=tf([[[1]]],[[[1,1]]]); print(g+g)"

   2 s + 2
-------------
s^2 + 2 s + 1

murrayrm · 2017-12-27T16:27:18Z

A few additional comments on this:

The same changes made here for xferfcn.py also need to be made in statesp.py and probably any other code that checks for data types int or float (need to add numpy types).
A quicker way to check against all of the numpy data types is to check against numpy.number.
Some related (and conflicting) changes are present in PR Fixes for SciPy 1.0 compatibility #170, though this PR goes beyond that in cleaning things up.

I'll try to do an integration of all of this in the coming days.

…n-control into jed-frey-InputSanitization

murrayrm · 2017-12-29T23:43:25Z

I submitted a pull request back to @jed-frey that addresses the issues above and also merges in the changes from PR #170, which are related, and the additional unit tests from PR #158 (also related). Will wait a few days fro @jed-frey to integrate the changes, otherwise I'll submit a separate pull request with the combined changes. Travis CI results available here (all OK).

Updates to input sanitization

Jed added 5 commits February 17, 2017 13:57

Added notebook for doing development work on IO-Cleanup.

37b06f4

Added IO cleanup function to xferfcn module.

Added .eggs directory to gitignore.

feb68f5

From .eggs/README.txt: "This directory contains eggs that were downloaded by setuptools to build, test, and run plug-ins. This directory caches those eggs to prevent repeated downloads. However, it is safe to delete this directory."

Broke out the numerator/denominator cleanup function into a separate …

5e2d95d

…function. Added unit tests to test the cleanup function. Added deep copy to TransferFunction args.

Merge remote-tracking branch 'upstream/master'

cd7f596

Removed test notebook

509ff8e

murrayrm reviewed Feb 18, 2017

View reviewed changes

Renamed cleanPart to _cleanPart since it is an internal function, as …

d44489f

…suggested by @murrayrm

roryyorke reviewed Mar 17, 2017

View reviewed changes

roryyorke mentioned this pull request Oct 22, 2017

SISO tf() may not work with numpy arrays with numpy.int elements. #158

Closed

murrayrm self-assigned this Dec 27, 2017

murrayrm added this to the 0.8.0 milestone Dec 27, 2017

murrayrm added 5 commits December 29, 2017 08:17

unit test from kangwonlee commit facfe9c

b1437f0

TST: renamed unit test from kangwonlee

10cdb7a

Merge branch 'InputSanitization' of https://github.com/jed-frey/pytho…

af8837e

…n-control into jed-frey-InputSanitization

Merge branch 'jed-frey-InputSanitization' into sstf_input_sanitization

b697a08

address additional comments from roryyorke in PR #135

68b5dd1

This was referenced Dec 30, 2017

Convert TransferFunction's shallow copy to deep copy. #130

Closed

tf should copy its arguments; instead they are mutated #112

Closed

Jed Frey and others added 3 commits December 31, 2017 11:56

Merge pull request #1 from murrayrm/sstf_input_sanitization

11a6264

Updates to input sanitization

Merge branch 'master' into InputSanitization

bbb902f

Merge branch 'master' into InputSanitization

8c997ff

murrayrm merged commit a5094e2 into python-control:master Jan 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup TransferFunction Input Sanitization #135

Cleanup TransferFunction Input Sanitization #135

dapperfu commented Feb 18, 2017

coveralls commented Feb 18, 2017 •

edited

Loading

murrayrm Feb 18, 2017

dapperfu Feb 18, 2017

dapperfu commented Feb 18, 2017

coveralls commented Feb 18, 2017 •

edited

Loading

roryyorke left a comment

roryyorke Mar 17, 2017

roryyorke Mar 17, 2017

roryyorke Mar 17, 2017

dapperfu Jun 26, 2017

roryyorke Mar 17, 2017

roryyorke Mar 17, 2017

roryyorke commented Jun 24, 2017

murrayrm commented Dec 27, 2017

murrayrm commented Dec 29, 2017 •

edited

Loading

Cleanup TransferFunction Input Sanitization #135

Cleanup TransferFunction Input Sanitization #135

Conversation

dapperfu commented Feb 18, 2017

Deep copy

Before:

After:

Validate All Input Types:

Before:

After

Added more valid types.

Before:

After:

Added Tuple as valid collection for MIMO systems

coveralls commented Feb 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dapperfu commented Feb 18, 2017

coveralls commented Feb 18, 2017 • edited Loading

roryyorke 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

roryyorke commented Jun 24, 2017

murrayrm commented Dec 27, 2017

murrayrm commented Dec 29, 2017 • edited Loading

coveralls commented Feb 18, 2017 •

edited

Loading

coveralls commented Feb 18, 2017 •

edited

Loading

murrayrm commented Dec 29, 2017 •

edited

Loading