- Notifications
You must be signed in to change notification settings - Fork441
Refactor the test suite using pytest for array and matrix types#438
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
coveralls commentedJul 25, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
| 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) |
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.
| try: | ||
| tf2*tf3 # Error; incompatible timebases | ||
| raise ValueError("incompatible operation allowed") | ||
| except ValueError: | ||
| pass | ||
| try: |
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.
| # Make sure that we get a pending deprecation warning | ||
| self.assertRaises(PendingDeprecationWarning, frd_tf.evalfr, 1.) | ||
| with pytest.deprecated_call(): | ||
| frd_tf.evalfr(1.) | ||
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
| @@ -1,393 +0,0 @@ | |||
| import unittest | |||
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.
9500543 tob7b7f7bCompare273a80b toe7affffCompare@sawyerbfuller, can I pick8e3ddd2 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. |
Hi ben , I agree that pull request will need to happen after the changesyou’ve been making. Right now though, that branch Has changes that I don’tthink are the right way to do it (incorrectly assumes omega is real, and Ithink 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 pick8e3ddd2 <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 FullerAssistant Professor of Mechanical EngineeringUniversity of Washingtonhttp://faculty.washington.edu/~minster/(Typed with my thumbs) |
I agree with this statement. |
90582e5 toda1c517Comparee96f365 toab77e02Compareremove a lot of duplicate code by converting everything intoa single parametrized test function.
7674d30 todeeb0c7CompareRebase 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. |
revert this commit when merging a rebasedpython-control#431(remove statesp_test.py::test_copy_constructor_nodt if not applicable)
deeb0c7 to2b98769CompareThere 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.
| - "3.8" | ||
| - "3.7" |
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.
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)
| @@ -4,5 +4,4 @@ universal=1 | |||
| [tool:pytest] | |||
| filterwarnings = | |||
| ignore:.*matrix subclass:PendingDeprecationWarning | |||
| ignore:.*scipy:DeprecationWarning | |||
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.
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 warningscontrol/tests/canonical_test.py: 180 warningscontrol/tests/config_test.py: 8 warningscontrol/tests/convert_test.py: 3080 warningscontrol/tests/discrete_test.py: 964 warningscontrol/tests/flatsys_test.py: 54 warningscontrol/tests/frd_test.py: 87 warningscontrol/tests/freqresp_test.py: 84 warningscontrol/tests/input_element_int_test.py: 8 warningscontrol/tests/iosys_test.py: 728 warningscontrol/tests/lti_test.py: 8 warningscontrol/tests/margin_test.py: 20 warningscontrol/tests/mateqn_test.py: 30 warningscontrol/tests/matlab2_test.py: 96 warningscontrol/tests/matlab_test.py: 896 warningscontrol/tests/minreal_test.py: 4496 warningscontrol/tests/modelsimp_test.py: 36 warningscontrol/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_inconsistentcontrol/tests/iosys_test.py::TestIOSys::test_named_signals_linearize_inconsistentcontrol/tests/iosys_test.py::TestIOSys::test_named_signals_linearize_inconsistentcontrol/tests/iosys_test.py::TestIOSys::test_named_signals_linearize_inconsistentcontrol/tests/matlab_test.py::TestMatlab::testDcgaincontrol/tests/matlab_test.py::TestMatlab::testDcgaincontrol/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.htmlThese warnings vanish, when I activatePYTHON_CONTROL_ARRAY_AND_MATRIX=1. Seems like the default when this is not set is wrong. Will track it down in a few hours.
| Programming Language :: Python :: 3.6 | ||
| Programming Language :: Python :: 3.7 | ||
| Programming Language :: Python :: 3.8 |
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).
…" frompython-control#438This reverts commit2b98769.
Uh oh!
There was an error while loading.Please reload this page.
Inspired by the discussion in#433 I created a pytest fixture
matarrayoutthat parametrizes all the test functions and runs them both withuse_numpy_matrix(True)and...(False)for the two availableoutput conventions. To turn on the autouse fixture, export the environment variablePYTHON_CONTROL_ARRAY_AND_MATRIX=1. There is an additional Travis CI job for this.It turned out, that the
unittest.TestCaseinheriting 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
config.reset_defaults()working against the functionality of the new fixture. So I changed that too and created the fixtureeditsdefaults. BTW, I thinkdefaultsis a misnomer. It should besettingsthat can be reset to default values.robust_array_test.pyis removed and its functionality completely replaced by the usage ofmatarrayininrobust_test.pystatefbk_array_test.pyandstate_fbk_matrix_test.pyare merged intostatefbk_test.pywithmatarrayin.statesp_array_test.pyis merged intostatesp_test.pytimeresp_test.pylooks 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 indo not override squeeze parameter in forced_response #442.xferfcn_test.py::test_matrix_multiply()now checks both left and right multiplication withnp.matrixand is prepared for array matmul, if we ever succeed to implement it (currently xfails for array).control.tests.conftestwhich can be used as decorators on tests for easier definition of skips:@slycotonly@nopython2@noscipy0matarrayoutfixture updates the warnings filter for numpy matrix deprecation warnings. No global filter anymore, so unexpected usage gets revealed formatarrayout=np.array.matarrayin. The newstatefbk_test.pymakes use of this -- No test code should directly use deprecatednp.matrixanymore.