NotificationsYou must be signed in to change notification settings
Fork441
Star1.8k

Cleanup TransferFunction Input Sanitization#135

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

murrayrm merged 14 commits intopython-control:masterfromdapperfu:InputSanitization

Jan 2, 2018

Merged

Cleanup TransferFunction Input Sanitization#135

murrayrm merged 14 commits intopython-control:masterfromdapperfu:InputSanitization

Jan 2, 2018

Conversation

Copy link

Contributor

dapperfu commentedFeb 18, 2017

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 (forSISO), 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 toint64 on 64-bit systems. It now accepts valid types ofint, int8, int16, int32, int64, float, float16, float32, float64, float128. (If for some reason your transfer function parts were generated in another function).

Code:

import controlfrom numpy import array, intnum = 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 (forSISO), 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 1num11=[1]den11=[2, 3]# Input 1 to Output 2num12=[4]den12=[5, 6]# Input 2 to Output 1num21=[7]den21=[8, 9]# Input 2 to Output 2num22=[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]))

Jed added5 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

Copy link

coveralls commentedFeb 18, 2017•
edited
Loading

Coverage increased (+0.08%) to 77.408% when pulling509ff8e on jed-frey:InputSanitization into5ab74cf on python-control:master.

murrayrm reviewed

Feb 18, 2017

View reviewed changes

control/xferfcn.py Outdated

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

		return (tf.num, tf.den)

		def cleanPart(data):

Copy link

Member

murrayrmFeb 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Since this is an internal function, should we name it_cleanPart?

Copy link

ContributorAuthor

dapperfuFeb 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

dapperfu commentedFeb 18, 2017

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

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

d44489f

…suggested by@murrayrm

Copy link

coveralls commentedFeb 18, 2017•
edited
Loading

Coverage increased (+0.07%) to 77.4% when pullingd44489f on jed-frey:InputSanitization into5ab74cf on python-control:master.

roryyorke reviewed

Mar 17, 2017

View reviewed changes

Copy link

Contributor

roryyorke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Looks good! Comments are all nitpicks.

control/tests/xferfcn_input_test.py Outdated

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

Copy link

Contributor

roryyorkeMar 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Probably worthwhile correcting this header comment

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."""

Copy link

Contributor

roryyorkeMar 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

floats, not ints?

control/xferfcn.py Outdated

		@@ -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

Copy link

Contributor

roryyorkeMar 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

ContributorAuthor

dapperfuJun 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

control/xferfcn.py Outdated

		data: numerator or denominator of a transfer function.

		Returns:
		data: correctly formatted transfer function part.

Copy link

Contributor

roryyorkeMar 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

control/xferfcn.py Outdated

		float, float16, float32, float64, float128)
		valid_collection = (list, tuple, ndarray)

		if (isinstance(data, valid_types) or

Copy link

Contributor

roryyorkeMar 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I was going to suggest usingnp.isreal instead ofisinstance(..,valid_types), but then I foundnp.isreal(file('/dev/null')) returns True (oops!).

Copy link

Contributor

roryyorke commentedJun 24, 2017

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 (forSISO), 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

roryyorke mentioned this pull request

Oct 22, 2017

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

Closed

Copy link

Member

murrayrm commentedDec 27, 2017

A few additional comments on this:

The same changes made here forxferfcn.py also need to be made instatesp.py and probably any other code that checks for data typesint orfloat (need to addnumpy types).
A quicker way to check against all of thenumpy data types is to check againstnumpy.number.
Some related (and conflicting) changes are present in PRFixes 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.

murrayrm self-assigned this

Dec 27, 2017

murrayrm added this to the0.8.0 milestone

Dec 27, 2017

murrayrm added5 commits

December 29, 2017 08:17

unit test from kangwonlee commitfacfe9c

b1437f0

TST: renamed unit test from kangwonlee

10cdb7a

Merge branch 'InputSanitization' ofhttps://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

Copy link

Member

murrayrm commentedDec 29, 2017•
edited
Loading

I submitted apull 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 availablehere (all OK).

This was referencedDec 30, 2017

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

Closed

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

Closed

Jed Freyand others added3 commits

December 31, 2017 11:56

Merge pull request#1from murrayrm/sstf_input_sanitization

11a6264

Updates to input sanitization

Merge branch 'master' into InputSanitization

bbb902f

Merge branch 'master' into InputSanitization

8c997ff

murrayrm merged commita5094e2 intopython-control:master

Jan 2, 2018

Labels

None yet

4 participants

Movatterモバイル変換

Cleanup TransferFunction Input Sanitization#135

Cleanup TransferFunction Input Sanitization#135

Uh oh!

Conversation

dapperfu commentedFeb 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

Uh oh!

coveralls commentedFeb 18, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dapperfu commentedFeb 18, 2017

Uh oh!

coveralls commentedFeb 18, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

roryyorke 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!

roryyorke commentedJun 24, 2017

Uh oh!

murrayrm commentedDec 27, 2017

Uh oh!

murrayrm commentedDec 29, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Uh oh!

coveralls commentedFeb 18, 2017•
edited
Loading

coveralls commentedFeb 18, 2017•
edited
Loading

murrayrm commentedDec 29, 2017•
edited
Loading