-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Contouring 1x1 array (issue #8197) #8227
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
lib/matplotlib/tests/test_contour.py
Outdated
try: | ||
plt.contour([[0]]) | ||
except TypeError as exc: | ||
assert exc.args[0] == 'Input z must be at least a 2x2 array.' |
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.
we have recently changed over to using py.test. You can see how asserts are tested here: http://doc.pytest.org/en/latest/assert.html#assertions-about-expected-exceptions
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.
Exception-catching tests changed to pytest, including those elsewhere in test_contour.py. Thanks for the link @WeatherGod.
@@ -309,3 +309,53 @@ def test_contourf_symmetric_locator(): | |||
locator = plt.MaxNLocator(nbins=4, symmetric=True) | |||
cs = plt.contourf(z, locator=locator) | |||
assert_array_almost_equal(cs.levels, np.linspace(-12, 12, 5)) | |||
|
|||
|
|||
def test_contour_1x1_array(): |
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 test might need a cleanup decorator, because while the plt.contour()
calls all fail, figures and axes are still produced prior to throwing the error.
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.
master
does not need or use the cleanup
decorator any more; it's all in the py.test
fixture.
pytest.ini
Outdated
@@ -50,11 +50,13 @@ pep8ignore = | |||
matplotlib/testing/jpl_units/UnitDbl.py E201 E202 E203 | |||
matplotlib/testing/jpl_units/UnitDblConverter.py E201 E202 E203 E251 E302 E501 E711 | |||
matplotlib/testing/jpl_units/UnitDblFormatter.py E201 E202 E251 E302 | |||
matplotlib/tests/test_contour.py E501 |
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 PEP8 exceptions list should only get smaller, not bigger. Probably will be fixed by using the py.test
method noted by @WeatherGod.
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.
@QuLogic: done.
Should I squash these commits into one? |
@ianthomas23 if you're comfortable with that workflow, go for it. |
d716c7b
to
3e23c4e
Compare
Squashed commits. |
Is this worth backporting to 2.0.x? This seems like a pretty narrow corner that has been broken for a long time? |
I would say "no" to backporting. |
There are two underlying problems here. Firstly, we've never bothered to explicitly check if contour/contourf are called with a 2D array that is smaller than 2x2, but have instead relied upon later code to raise an error. There are different code paths for the legacy and new contouring algorithms, hence two different errors reported by OP. Secondly, in the new contour code (and also tricontour too) I haven't correctly handled all errors raised; I've called PyErr_SetString in each case but not necessarily returned -1 or NULL (depending on context) correctly. This shouldn't really matter as all such C++ code is private and covered by checks in the python wrappers - well it should be be, but evidently isn't in this case.
Hence there are two parts to the fix. Firstly, I've added explicit checks in the python contour code for arrays smaller than 2x2, and this is done twice as there are 2 code paths. I've also put a new test in test_contour.py to test that these errors are correctly raised. Secondly, I've corrected _contour_wrapper.cpp and _tri_wrapper.cpp to correctly report errors, and added a set of tests for each so that each raising of a python error in the C++ code is tested at least once.