Unit tests for gangof4_plot #505

murrayrm · 2021-01-13T01:20:10Z

This PR adds a unit test for the gang of 4 plots, resolving issue #500.

It also tests whether the new GitHub Actions CI interface is working...

This reverts commit d0f29fc.

coveralls · 2021-01-13T01:24:10Z

Coverage increased (+1.004%) to 88.543% when pulling b300886 on murrayrm:unit_tests_gangof4 into 1e6da71 on python-control:master.

bnavigator · 2021-01-13T01:28:23Z

control/tests/baseline_images/freqresp_test/gangof4-pvtol-expected.png

@@ -0,0 +1 @@
+/Users/murray/Dropbox/macosx/src/python-control/murrayrm/control/tests/baseline_images/freqresp_test/gangof4-pvtol.png


That's very odd. Should have been a copy of the image...

Symbolic link set up by pytest that apparently used an absolute path. Fixed.

pytest.ini

bnavigator · 2021-01-13T01:30:44Z

control/tests/freqresp_test.py

+    return (Pi, Ci)
+
+# Regression test for Gang of 4 plots
+@nopython2


Why install nose in python2 then? Just for the import?

Exactly: the problem I was having is that I need to import image_comparison from matplotlib.testing.decorators and this requires nose under python2 (no idea why). And I think there was some problem with getting the comparison to work in Python2 (at some point), so I skipped the unit test.

We can probably clean it up, but since it might break Travis and since we are deprecating Python 2, I'm going to leave for now.

I hope we can get rid of Python 2 soon.

Thanks for the effort, but... in my experience image comparisons with matplotlib outputs are very brittle.
Whenever matplotlib decides to change their defaults (which they have done in the past), or a font is ever so slightly different, the test will fail. It's a never ending maintenance hassle.

Probably better to mock the matplotlib interface and only check for the right interface calls to mpl.
I'm willing to help.

To help you on your way:

from unittest.mock import patch def test_gangof4_pvtol(pvtol_inner): with patch('control.freqplot.plt') as mockplt: ctrl.gangof4(*pvtol_inner) print(mockplt.mock_calls)

The output of this looks something like this:

call.gcf().axes.__iter__(), call.clf(), call.subplot(221, label='control-gangof4-s'), call.subplot(222, label='control-gangof4-ps'), call.subplot(223, label='control-gangof4-cs'), call.subplot(224, label='control-gangof4-t'), call.subplot().loglog(array([1.00000000e-01, 1.20679264e-01, 1.45634848e-01, 1.75751062e-01, 2.12095089e-01, 2.55954792e-01, 3.08884360e-01, 3.72759372e-01, 4.49843267e-01, 5.42867544e-01, 6.55128557e-01, 7.90604321e-01, 9.54095476e-01, 1.15139540e+00, 1.38949549e+00, 1.67683294e+00, 2.02358965e+00, 2.44205309e+00, 2.94705170e+00, 3.55648031e+00, 4.29193426e+00, 5.17947468e+00, 6.25055193e+00, 7.54312006e+00, 9.10298178e+00, 1.09854114e+01, 1.32571137e+01, 1.59985872e+01, 1.93069773e+01, 2.32995181e+01, 2.81176870e+01, 3.39322177e+01, 4.09491506e+01, 4.94171336e+01, 5.96362332e+01, 7.19685673e+01, 8.68511374e+01, 1.04811313e+02, 1.26485522e+02, 1.52641797e+02, 1.84206997e+02, 2.22299648e+02, 2.68269580e+02, 3.23745754e+02, 3.90693994e+02, 4.71486636e+02, 5.68986603e+02, 6.86648845e+02, 8.28642773e+02, 1.00000000e+03]), array([2.37260369e-04, 3.45374869e-04, 5.02649698e-04, 7.31321247e-04, 1.06355413e-03, 1.54573109e-03, 2.24444103e-03, 3.25466960e-03, 4.71066442e-03, 6.79967979e-03, 9.77807906e-03, 1.39878108e-02, 1.98689733e-02, 2.79616651e-02, 3.88897504e-02, 5.33242435e-02, 7.19369207e-02, 9.53699704e-02, 1.24250287e-01, 1.59257279e-01, 2.01222831e-01, 2.51226612e-01, 3.10656635e-01, 3.81214259e-01, 4.64827654e-01, 5.63376984e-01, 6.78010385e-01, 8.07663375e-01, 9.46435949e-01, 1.08059131e+00, 1.18899261e+00, 1.25212430e+00, 1.26542868e+00, 1.24175520e+00, 1.20028019e+00, 1.15590007e+00, 1.11647717e+00, 1.08468611e+00, 1.06045859e+00, 1.04263298e+00, 1.02981186e+00, 1.02072737e+00, 1.01435479e+00, 1.00991477e+00, 1.00683547e+00, 1.00470657e+00, 1.00323791e+00, 1.00222621e+00, 1.00152999e+00, 1.00105121e+00])), call.subplot().set_ylabel(''), call.subplot().tick_params(labelbottom=False), call.subplot().grid(True, which='both'), call.subplot().loglog(array([1.00000000e-01, 1.20679264e-01, 1.45634848e-01, 1.75751062e-01, 2.12095089e-01, 2.55954792e-01, 3.08884360e-01, 3.72759372e-01, 4.49843267e-01, 5.42867544e-01, 6.55128557e-01, 7.90604321e-01, 9.54095476e-01, 1.15139540e+00, 1.38949549e+00, 1.67683294e+00, 2.02358965e+00, 2.44205309e+00, 2.94705170e+00, 3.55648031e+00, 4.29193426e+00, 5.17947468e+00, 6.25055193e+00, 7.54312006e+00, 9.10298178e+00, 1.09854114e+01, 1.32571137e+01, 1.59985872e+01, 1.93069773e+01, 2.32995181e+01, 2.81176870e+01, 3.39322177e+01, 4.09491506e+01, 4.94171336e+01, 5.96362332e+01, 7.19685673e+01, 8.68511374e+01, 1.04811313e+02, 1.26485522e+02, 1.52641797e+02, 1.84206997e+02, 2.22299648e+02, 2.68269580e+02, 3.23745754e+02, 3.90693994e+02, 4.71486636e+02, 5.68986603e+02, 6.86648845e+02, 8.28642773e+02, 1.00000000e+03]), array([1.24873879e-01, 1.24816450e-01, 1.24732955e-01, 1.24611657e-01, 1.24435636e-01, 1.24180616e-01, 1.23812007e-01, 1.23281004e-01, 1.22519754e-01, 1.21435903e-01, 1.19907647e-01, 1.17781690e-01, 1.14878340e-01, 1.11009578e-01, 1.06014988e-01, 9.98140114e-02, 9.24598842e-02, 8.41681618e-02, 7.52953730e-02, 6.62681540e-02, 5.74933390e-02, 4.92879337e-02, 4.18495024e-02, 3.52625322e-02, 2.95237153e-02, 2.45704351e-02, 2.03041487e-02, 1.66078542e-02, 1.33631565e-02, 1.04764464e-02, 7.91528160e-03, 5.72360105e-03, 3.97186315e-03, 2.67625372e-03, 1.77626855e-03, 1.17457560e-03, 7.79013934e-04, 5.19677785e-04, 3.48865868e-04, 2.35521695e-04, 1.59732041e-04, 1.08712281e-04, 7.41811267e-05, 5.07134269e-05, 3.47161403e-05, 2.37873940e-05, 1.63097105e-05, 1.11877503e-05, 7.67671937e-06, 5.26869060e-06])), call.subplot().tick_params(labelbottom=False), call.subplot().set_ylabel(''), call.subplot().grid(True, which='both'), call.subplot().loglog(array([1.00000000e-01, 1.20679264e-01, 1.45634848e-01, 1.75751062e-01, 2.12095089e-01, 2.55954792e-01, 3.08884360e-01, 3.72759372e-01, 4.49843267e-01, 5.42867544e-01, 6.55128557e-01, 7.90604321e-01, 9.54095476e-01, 1.15139540e+00, 1.38949549e+00, 1.67683294e+00, 2.02358965e+00, 2.44205309e+00, 2.94705170e+00, 3.55648031e+00, 4.29193426e+00, 5.17947468e+00, 6.25055193e+00, 7.54312006e+00, 9.10298178e+00, 1.09854114e+01, 1.32571137e+01, 1.59985872e+01, 1.93069773e+01, 2.32995181e+01, 2.81176870e+01, 3.39322177e+01, 4.09491506e+01, 4.94171336e+01, 5.96362332e+01, 7.19685673e+01, 8.68511374e+01, 1.04811313e+02, 1.26485522e+02, 1.52641797e+02, 1.84206997e+02, 2.22299648e+02, 2.68269580e+02, 3.23745754e+02, 3.90693994e+02, 4.71486636e+02, 5.68986603e+02, 6.86648845e+02, 8.28642773e+02, 1.00000000e+03]), array([1.90045028e-03, 2.76801618e-03, 4.03182734e-03, 5.87307962e-03, 8.55606554e-03, 1.24665394e-02, 1.81680607e-02, 2.64849951e-02, 3.86252369e-02, 5.63624073e-02, 8.23073431e-02, 1.20313377e-01, 1.76080109e-01, 2.58045753e-01, 3.78687011e-01, 5.56378770e-01, 8.18017500e-01, 1.20271551e+00, 1.76706084e+00, 2.59269630e+00, 3.79723689e+00, 5.54974746e+00, 8.09203498e+00, 1.17664714e+01, 1.70487195e+01, 2.45764570e+01, 3.51462684e+01, 4.96104601e+01, 6.85493958e+01, 9.16201542e+01, 1.16854860e+02, 1.40868317e+02, 1.60548552e+02, 1.74721045e+02, 1.84058879e+02, 1.89930635e+02, 1.93569151e+02, 1.95834494e+02, 1.97264771e+02, 1.98183002e+02, 1.98781882e+02, 1.99177719e+02, 1.99442118e+02, 1.99620129e+02, 1.99740677e+02, 1.99822653e+02, 1.99878563e+02, 1.99916776e+02, 1.99942930e+02, 1.99960848e+02])), call.subplot().set_xlabel('Frequency (rad/sec)'), call.subplot().set_ylabel(''), call.subplot().grid(True, which='both'), call.subplot().loglog(array([1.00000000e-01, 1.20679264e-01, 1.45634848e-01, 1.75751062e-01, 2.12095089e-01, 2.55954792e-01, 3.08884360e-01, 3.72759372e-01, 4.49843267e-01, 5.42867544e-01, 6.55128557e-01, 7.90604321e-01, 9.54095476e-01, 1.15139540e+00, 1.38949549e+00, 1.67683294e+00, 2.02358965e+00, 2.44205309e+00, 2.94705170e+00, 3.55648031e+00, 4.29193426e+00, 5.17947468e+00, 6.25055193e+00, 7.54312006e+00, 9.10298178e+00, 1.09854114e+01, 1.32571137e+01, 1.59985872e+01, 1.93069773e+01, 2.32995181e+01, 2.81176870e+01, 3.39322177e+01, 4.09491506e+01, 4.94171336e+01, 5.96362332e+01, 7.19685673e+01, 8.68511374e+01, 1.04811313e+02, 1.26485522e+02, 1.52641797e+02, 1.84206997e+02, 2.22299648e+02, 2.68269580e+02, 3.23745754e+02, 3.90693994e+02, 4.71486636e+02, 5.68986603e+02, 6.86648845e+02, 8.28642773e+02, 1.00000000e+03]), array([1.00023699e+00, 1.00034480e+00, 1.00050143e+00, 1.00072873e+00, 1.00105808e+00, 1.00153419e+00, 1.00222016e+00, 1.00320376e+00, 1.00460447e+00, 1.00657972e+00, 1.00932707e+00, 1.01307581e+00, 1.01805918e+00, 1.02445794e+00, 1.03231568e+00, 1.04144744e+00, 1.05139061e+00, 1.06144894e+00, 1.07083459e+00, 1.07884047e+00, 1.08494561e+00, 1.08880020e+00, 1.09010270e+00, 1.08840519e+00, 1.08285627e+00, 1.07184755e+00, 1.05251347e+00, 1.02013204e+00, 9.67879872e-01, 8.88267024e-01, 7.77918313e-01, 6.43924924e-01, 5.03921624e-01, 3.76562022e-01, 2.72384733e-01, 1.92999287e-01, 1.35061486e-01, 9.38251491e-02, 6.48954574e-02, 4.47678113e-02, 3.08326570e-02, 2.12133669e-02, 1.45854697e-02, 1.00240348e-02, 6.88714850e-03, 4.73099342e-03, 3.24944011e-03, 2.23165086e-03, 1.53256095e-03, 1.05242552e-03])), call.subplot().set_xlabel('Frequency (rad/sec)'), call.subplot().set_ylabel(''), call.subplot().grid(True, which='both'), call.tight_layout()]

Verifying this list should be suitable for unittesting.

Comparing with a fixed set of samples with values of arbitrary precision is equally brittle.

The issue to test here is not about the correct value of the curves but that the plot functions produce a non-crashing and legible plot.

@bnavigator
Respectfully disagree, we are not testing matplotlib, we must make sure that we send the right calls to matplotlib.

And we wouldn't compare the calls with a hard coded array, but with the data from within the gangof4 function itself, using float compares with tolerances.

I'll support your decision.

Sorry, I still don't see a point in testing that the calls which are defined in the source code are the same calls which we retype again in the test code. There is nothing meaningful to compare.

…up.cfg

bnavigator · 2021-01-13T01:54:38Z

I don't think the symlinked file is needed at all.
The comparison test fails on my machine:

expected:

actual:

diff:

Comes from float data, needs float tolerance?

E           matplotlib.testing.exceptions.ImageComparisonFailure: images not close (RMS 14.842):
E               result_images/freqresp_test/gangof4-pvtol.png
E               result_images/freqresp_test/gangof4-pvtol-expected.png
E               result_images/freqresp_test/gangof4-pvtol-failed-diff.png

murrayrm · 2021-01-14T18:52:22Z

It looks like bitmap comparison is not going to work here (the problem is that small shifts are causing large differences in the bitmap). I'll have to write something that extracts individual elements and make sure they are in the right places.

bnavigator · 2021-01-14T18:57:54Z

I am not sure if it is really worth the effort. My suggestion was just an idea.

Would it suffice to

remove the axis text by the appropriate keyword of the tester,
switching off the grid, and
a large enough resolution, (which would not increase the file size too much because the png compression should work good enough)?

murrayrm · 2024-08-04T19:51:32Z

Getting rid of this (very old) PR. There is now a script examples/plot_gallery.py that generates a set of "standard" plots that can be used to identify changes (by eye). Probably OK for now.

murrayrm added 7 commits January 9, 2021 19:13

Create python-package.yml

d0f29fc

Merge branch 'master' of https://github.com/murrayrm/python-control

2d44387

Merge branch 'master' of https://github.com/python-control/python-con…

97c71d6

…trol

Revert "Create python-package.yml"

a64f7f1

This reverts commit d0f29fc.

add gangof4 unit test using mpl.image_comparison

362f46d

add pytest.ini file with customer marker to avoid mpl warning

518c061

import nose to avoid Python 2.7/matplotlib.testing error

2d1650d

murrayrm linked an issue Jan 13, 2021 that may be closed by this pull request

gangof4_plot() is not covered by unit tests. #500

Open

bnavigator reviewed Jan 13, 2021

View reviewed changes

pytest.ini Outdated Show resolved Hide resolved

bnavigator reviewed Jan 13, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

address bnavigator review comments: image symlink + pytest.ini -> set…

b300886

…up.cfg

bnavigator mentioned this pull request Jan 13, 2021

Move from legacy Travis CI integration to a Github Actions based system #446

Closed

bnavigator added the needs rebase The PR needs a rebase to current master branch label Mar 8, 2021

murrayrm closed this Aug 4, 2024

murrayrm added this to the 0.10.1 milestone Aug 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unit tests for gangof4_plot #505

Unit tests for gangof4_plot #505

murrayrm commented Jan 13, 2021

coveralls commented Jan 13, 2021 •

edited

Loading

bnavigator Jan 13, 2021

murrayrm Jan 13, 2021

murrayrm Jan 13, 2021

bnavigator Jan 13, 2021

murrayrm Jan 13, 2021 •

edited

Loading

bnavigator Jan 13, 2021

henklaak Feb 2, 2023 •

edited

Loading

henklaak Feb 2, 2023 •

edited

Loading

bnavigator Feb 2, 2023

bnavigator Feb 2, 2023

henklaak Feb 2, 2023

bnavigator Feb 2, 2023

This comment has been minimized.

bnavigator commented Jan 13, 2021 •

edited

Loading

murrayrm commented Jan 14, 2021

bnavigator commented Jan 14, 2021

murrayrm commented Aug 4, 2024

		@@ -0,0 +1 @@
		/Users/murray/Dropbox/macosx/src/python-control/murrayrm/control/tests/baseline_images/freqresp_test/gangof4-pvtol.png

Unit tests for gangof4_plot #505

Unit tests for gangof4_plot #505

Conversation

murrayrm commented Jan 13, 2021

coveralls commented Jan 13, 2021 • edited Loading

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

murrayrm Jan 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henklaak Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

henklaak Feb 2, 2023 • edited Loading

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

This comment has been minimized.

bnavigator commented Jan 13, 2021 • edited Loading

murrayrm commented Jan 14, 2021

bnavigator commented Jan 14, 2021

murrayrm commented Aug 4, 2024

coveralls commented Jan 13, 2021 •

edited

Loading

murrayrm Jan 13, 2021 •

edited

Loading

henklaak Feb 2, 2023 •

edited

Loading

henklaak Feb 2, 2023 •

edited

Loading

bnavigator commented Jan 13, 2021 •

edited

Loading