GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Refactor the test suite using pytest for array and matrix types by bnavigator · Pull Request #438 · python-control/python-control

Conversation

bnavigator commented Jul 25, 2020 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Inspired by the discussion in #433 I created a pytest fixture matarrayout that parametrizes all the test functions and runs them both with use_numpy_matrix(True) and ...(False) for the two available output conventions. To turn on the autouse fixture, export the environment variable PYTHON_CONTROL_ARRAY_AND_MATRIX=1. There is an additional Travis CI job for this.

It turned out, that the unittest.TestCase inheriting test classes don't like using the autoused fixture. So one thing let to the other and I converted all the test files to conform to pytest.

Test suite changes

There are several tests that use config.reset_defaults() working against the functionality of the new fixture. So I changed that too and created the fixture editsdefaults. BTW, I think defaults is a misnomer. It should be settings that can be reset to default values.
robust_array_test.py is removed and its functionality completely replaced by the usage of matarrayin in robust_test.py
statefbk_array_test.py and state_fbk_matrix_test.py are merged into statefbk_test.py with matarrayin.
statesp_array_test.py is merged into statesp_test.py
timeresp_test.py looks completeley different now. The tests are organized more systematically to test the time response functions with a more complete set of parameter variations. This revealed the issue described in do not override squeeze parameter in forced_response #442.
xferfcn_test.py::test_matrix_multiply() now checks both left and right multiplication with np.matrix and is prepared for array matmul, if we ever succeed to implement it (currently xfails for array).
the Travis CI test matrix is extended to Python 3.8 and the matrix definition is cleaned up.
There are a few new named pytest marks importable from control.tests.conftest which can be used as decorators on tests for easier definition of skips:
- @slycotonly
- @nopython2
- @noscipy0
The matarrayout fixture updates the warnings filter for numpy matrix deprecation warnings. No global filter anymore, so unexpected usage gets revealed for matarrayout=np.array.
For checking that functions work with matrix or arrays as input, you can use the fixture matarrayin. The new statefbk_test.py makes use of this -- No test code should directly use deprecated np.matrix anymore.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

coveralls commented Jul 25, 2020 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Coverage increased (+0.4%) to 87.04% when pulling 2b98769 on bnavigator:array-matrix-tests into 4eee1f3 on python-control:master.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bnavigator commented

Jul 25, 2020

View reviewed changes

Comment thread

control/tests/convert_test.py

		ssxfrm_real = ssxfrm_mag * np.cos(ssxfrm_phase)
		ssxfrm_imag = ssxfrm_mag * np.sin(ssxfrm_phase)
		np.testing.assert_array_almost_equal(
		ssorig_real, ssxfrm_real, decimal=5)

bnavigator Jul 25, 2020

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I had to reduce precision here, because running the test with matrix and array advances the random seed and produces different random systems.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bnavigator commented

Jul 25, 2020

View reviewed changes

Comment thread

control/tests/discrete_test.py

Comment on lines -131 to -136

		try:
		tf2*tf3 # Error; incompatible timebases
		raise ValueError("incompatible operation allowed")
		except ValueError:
		pass
		try:

bnavigator Jul 25, 2020

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The pytest code makes this look a lot cleaner.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sawyerbfuller reacted with thumbs up emoji

All reactions

👍 1 reaction

bnavigator commented

Jul 25, 2020

View reviewed changes

Comment thread

control/tests/frd_test.py

		self.assertRaises(PendingDeprecationWarning, frd_tf.evalfr, 1.)
		with pytest.deprecated_call():
		frd_tf.evalfr(1.)

bnavigator Jul 25, 2020

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Pytest takes care of this just fine

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bnavigator commented

Jul 25, 2020

View reviewed changes

Comment thread

control/tests/robust_array_test.py

		@@ -1,393 +0,0 @@
		import unittest

bnavigator Jul 25, 2020

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Same code as robust_test.py, just different setup and teardown. So obsolete with the new fixture.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bnavigator force-pushed the array-matrix-tests branch 2 times, most recently from 9500543 to b7b7f7b Compare

July 26, 2020 00:05

bnavigator commented Jul 26, 2020

Copy link

Copy Markdown

Contributor Author

the iosys_test now fails with incompatible timebases. probably need to merge #431 too. Has some merge conflicts, will need to rebase and apply #431, #433 in order...

sawyerbfuller reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bnavigator force-pushed the array-matrix-tests branch 3 times, most recently from 273a80b to e7affff Compare

July 26, 2020 14:31

bnavigator commented Jul 26, 2020

Copy link

Copy Markdown

Contributor Author

@sawyerbfuller, can I pick 8e3ddd2 from your branch fix-lti-evalfr into this PR and pytestify it right away? That way you don't have to submit a PR yourself and I don't have to rebase those fixes later.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sawyerbfuller commented Jul 26, 2020 via email

Copy link

Copy Markdown

Contributor

Hi ben , I agree that pull request will need to happen after the changes you’ve been making. Right now though, that branch Has changes that I don’t think are the right way to do it (incorrectly assumes omega is real, and I think we should move toward using __call__) so please don’t merge. S

On Sun, Jul 26, 2020 at 7:53 AM Ben Greiner ***@***.***> wrote: @sawyerbfuller <https://github.com/sawyerbfuller>, can I pick 8e3ddd2 <8e3ddd2> from your branch fix-lti-evalfr into this PR and pytestify it right away? That way you don't have to submit a PR yourself and I don't have to rebase those fixes later. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#438 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AN74SSOBSVQL4M4UFQGXYLTR5Q7L7ANCNFSM4PHU6UNA> .

-- Sawyer Fuller Assistant Professor of Mechanical Engineering University of Washington http://faculty.washington.edu/~minster/ (Typed with my thumbs)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bnavigator force-pushed the array-matrix-tests branch from 9804e7b to 8ee73b9 Compare

July 26, 2020 21:42

sawyerbfuller commented Jul 27, 2020

Copy link

Copy Markdown

Contributor

BTW, I think 'defaults' is a misnomer. It should be 'settings' that can be reset to default values.

I agree with this statement.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bnavigator mentioned this pull request

Jul 29, 2020

Return type of eigenvalues from lqr and lqe not consistent #439

Closed

bnavigator force-pushed the array-matrix-tests branch from 0bd120d to 45d677f Compare

July 29, 2020 21:12

This was referenced Jul 29, 2020

Complex matrices #376

Closed

do not override squeeze parameter in forced_response #442

Merged

bnavigator force-pushed the array-matrix-tests branch 4 times, most recently from 90582e5 to da1c517 Compare

August 1, 2020 13:46

This was referenced Aug 1, 2020

Fix matlab.stepinfo() #444

Merged

Fixed InterconnectedSystems name bugs. #400

Merged

bnavigator marked this pull request as ready for review

August 1, 2020 18:21

bnavigator added a commit to bnavigator/python-control that referenced this pull request

Aug 2, 2020


          cover TransferFunction.returnScipySignalLTI() (backported from python…

ba7817c

…-control#438)

bnavigator added a commit to bnavigator/python-control that referenced this pull request

Aug 2, 2020


          returnScipySignalLTI for discrete systems and add unit tests

a8aa41e

(backported from python-control#438)

bnavigator force-pushed the array-matrix-tests branch 2 times, most recently from e96f365 to ab77e02 Compare

August 2, 2020 22:55

bnavigator added 18 commits

December 30, 2020 00:56


          pytestify input_element_int_test

caed712


          pytestify iosys_test

55a6119


          pytestify lti_test.py

521e927


          pytestify mateqn_test

2c31029


          pytestify matlab tests

fc2f770


          pytestify minreal_test


          pytestify modelsimp

97eedaf


          pytestify nichols_test

3eb0a92


          pytestify phaseplot_test

000998f


          pytestify rlocus_test.py


          pytestify robust tests

f745bd2


          pytestify sisotool_test.py

2ee0773


          pytestify slycot_convert_test.py

e660363


          pytestify statefbk tests

952c511


          pytestify statesp tests


          pytestify timeresp_test.py

1dd2525


          pytestify xferfcn_input_test.py

29883e8

remove a lot of duplicate code by converting everything into
a single parametrized test function.


          pytestify xferfcn tests

ca9476f

bnavigator force-pushed the array-matrix-tests branch from 7674d30 to deeb0c7 Compare

December 29, 2020 23:56

bnavigator commented Dec 30, 2020

Copy link

Copy Markdown

Contributor Author

Rebase is done. I pulled everything not directly related to the testsuite out into different PRs. The new timevector calculation needs a minor change (#485) to fix the failing tests here.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bnavigator mentioned this pull request

Dec 30, 2020

deprecate np.matrix usage #486

Merged


          make tests work with pre python-control#431 source code state

2b98769

revert this commit when merging a rebased python-control#431

(remove statesp_test.py::test_copy_constructor_nodt if not applicable)

bnavigator force-pushed the array-matrix-tests branch from deeb0c7 to 2b98769 Compare

December 30, 2020 16:54

murrayrm reviewed

Dec 30, 2020

View reviewed changes

murrayrm left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

A few small things that could be fixed, but OK to merge.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

.travis.yml

Comment on lines +17 to 18

		- "3.8"
		- "3.7"

murrayrm Dec 30, 2020

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

To minimize CI resource usage, would it make sense to just check against the earliest supported Python 3 release and the latest stable Python 3 release? So 3.6 and 3.9, with the idea that if it works in those two then 3.7, and 3.8 are probably OK as well.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bnavigator Dec 30, 2020

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Definitely. AFAIK we do not make use of any of the changed Python stdlib changes like importlib-metadata, -resources, -dataclasses or the like.

We will need to revisit the test strategy matrix when moving from travis-ci.org to travis-ci.com or Github Actions. (#446)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

setup.cfg

		[tool:pytest]
		filterwarnings =
		ignore:.*matrix subclass:PendingDeprecationWarning
		ignore:.*scipy:DeprecationWarning

murrayrm Dec 30, 2020

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Do we really want to ignore these? Seems like something we would want to flag in a CI test.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bnavigator Dec 30, 2020

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This change removes one ignore. We can also remove the other and should track down any remaining matrix usage (in a separate PR, if outside of the test suite). Without the remaining filter, I get this summary:

============================================= warnings summary ==============================================
control/tests/bdalg_test.py: 387 warnings
control/tests/canonical_test.py: 180 warnings
control/tests/config_test.py: 8 warnings
control/tests/convert_test.py: 3080 warnings
control/tests/discrete_test.py: 964 warnings
control/tests/flatsys_test.py: 54 warnings
control/tests/frd_test.py: 87 warnings
control/tests/freqresp_test.py: 84 warnings
control/tests/input_element_int_test.py: 8 warnings
control/tests/iosys_test.py: 728 warnings
control/tests/lti_test.py: 8 warnings
control/tests/margin_test.py: 20 warnings
control/tests/mateqn_test.py: 30 warnings
control/tests/matlab2_test.py: 96 warnings
control/tests/matlab_test.py: 896 warnings
control/tests/minreal_test.py: 4496 warnings
control/tests/modelsimp_test.py: 36 warnings
control/tests/statefbk_test.py: 53 warnings
  /home/greiner/src/python-control/control/statesp.py:102: PendingDeprecationWarning: the matrix subclass is not the recommended way to represent matrices or deal with linear algebra (see https://docs.scipy.org/doc/numpy/user/numpy-for-matlab-users.html). Please adjust your code to use regular ndarray.
    arr = np.matrix(data, dtype=float)

control/tests/iosys_test.py::TestIOSys::test_named_signals_linearize_inconsistent
control/tests/iosys_test.py::TestIOSys::test_named_signals_linearize_inconsistent
control/tests/iosys_test.py::TestIOSys::test_named_signals_linearize_inconsistent
control/tests/iosys_test.py::TestIOSys::test_named_signals_linearize_inconsistent
control/tests/matlab_test.py::TestMatlab::testDcgain
control/tests/matlab_test.py::TestMatlab::testDcgain
control/tests/statefbk_test.py::TestStatefbk::test_LQE[arrayin]
control/tests/statefbk_test.py::TestStatefbk::test_LQE[arrayin]
  /usr/lib64/python3.8/site-packages/numpy/matrixlib/defmatrix.py:69: PendingDeprecationWarning: the matrix subclass is not the recommended way to represent matrices or deal with linear algebra (see https://docs.scipy.org/doc/numpy/user/numpy-for-matlab-users.html). Please adjust your code to use regular ndarray.
    return matrix(data, dtype=dtype, copy=False)

-- Docs: https://docs.pytest.org/en/stable/warnings.html

These warnings vanish, when I activate PYTHON_CONTROL_ARRAY_AND_MATRIX=1. Seems like the default when this is not set is wrong. Will track it down in a few hours.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

setup.py

Comment on lines 22 to 24

		Programming Language :: Python :: 3.6
		Programming Language :: Python :: 3.7
		Programming Language :: Python :: 3.8

murrayrm Dec 30, 2020

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Not sure whether we should only list versions we are explicitly testing against (so this would match .travis.yml list).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

murrayrm merged commit c432fd5 into python-control:master

Dec 30, 2020

murrayrm mentioned this pull request

Dec 30, 2020

reduce Python 3 testing to speed up Travis CI testing #487

Merged

bnavigator mentioned this pull request

Dec 30, 2020

Unit tests do still not use array but matrix by default #488

Closed

bnavigator added a commit to bnavigator/python-control that referenced this pull request

Dec 31, 2020


          Revert "make tests work with pre python-control#431 source code state…

578cfa6

…" from python-control#438

This reverts commit 2b98769.

bnavigator mentioned this pull request

Dec 31, 2020

Rebased #431: Default dt is 0 #490

Merged

bnavigator deleted the array-matrix-tests branch

February 18, 2024 20:29

Reviewers

murrayrm

murrayrm left review comments

sawyerbfuller

sawyerbfuller requested changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.9.0

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

bnavigator commented Jul 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test suite changes

Uh oh!

coveralls commented Jul 25, 2020 • edited Loading 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnavigator commented Jul 26, 2020

Uh oh!

bnavigator commented Jul 26, 2020

Uh oh!

sawyerbfuller commented Jul 26, 2020 via email

Uh oh!

sawyerbfuller commented Jul 27, 2020

Uh oh!

bnavigator commented Dec 30, 2020

Uh oh!

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

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bnavigator commented Jul 25, 2020 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

coveralls commented Jul 25, 2020 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.