converted assert into exception #3060

montefra · 2014-05-13T20:14:01Z

A few days I ago I was experimenting for a possible answer to this question. While trying

fig1, ax1 = plt.subplots()
fig2, ax2 = plt.subplots()
fig2.add_axes(ax1)

I stumbled into an AssertionError without any explanation (I had to get the doc of add_axes).
My understanding is that assert should be reserved for debugging, not to throw errors if the user give a wrong parameter. So I've converted them to exception.

I've also noticed that assert is used all over the places. If there are no strong opinions in the next weeks I'll to go through the code and convert them to exceptions.

montefra · 2014-05-13T20:20:46Z

in def draw_artist(self, a) there is an other assert: is a going to be available only after draw has been draw, or the user could call the method before draw? If the case is the latter, then I'll move it to exception.

WeatherGod · 2014-05-13T20:58:06Z

I like this post about assertions:

https://mail.python.org/pipermail/python-list/2013-November/660401.html

I wouldn't go hog-wild in converting the asserts into exceptions, but
perhaps we could see what sort of asserts can be converted (and
vice-versa). I don't have a particular opinion about this one yet.

montefra · 2014-05-13T21:07:37Z

Thanks for the link, I'll read it more carefully tomorrow. I agree about not going wild. Anyway these two points at the end

Never use them for testing user-supplied data, or for anything
where the check must take place under all circumstances.Don't use them for checking input arguments to public library

functions (private ones are okay) since you don't control the
caller and can't guarantee that it will never break the
function's contract.

convinced me even more that my PR make sense

WeatherGod · 2014-05-13T21:14:46Z

Indeed. To highlight two particular paragraphs that I find illuminating:


Opinions on assertions vary, because they can be a statement of
confidence about the correctness of the code. If you're certain that the
code is correct, then assertions are pointless, since they will never
fail and you can safely remove them. If you're certain the checks can
fail (e.g. when testing input data provided by the user), then you dare
not use assert since it may be compiled away and then your checks will be
skipped.

It's the situations in between those two that are interesting, times when
you're certain the code is correct but not *quite* absolutely certain.
Perhaps you've missed some odd corner case (we're all only human). In
this case an extra runtime check helps reassure you that any errors will
be caught as early as possible rather than in distant parts of the code.

On Tue, May 13, 2014 at 5:07 PM, Francesco Montesano <
notifications@github.com> wrote:

Thanks for the link, I'll read it more carefully tomorrow. I agree about
not going wild. Anyway these two points at the end

Never use them for testing user-supplied data, or for anything where
the check must take place under all circumstances.Don't use them for
checking input arguments to public library

functions (private ones are okay) since you don't control the caller
and can't guarantee that it will never break the function's contract.

convinced me even more that my PR make sense

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3060#issuecomment-43013027
.

tacaswell · 2014-05-17T13:33:27Z

I don't really want to do this for 1.4.0, labeled for 1.4.x

pelson · 2014-05-20T14:23:44Z

lib/matplotlib/figure.py

-            assert(a.get_figure() is self)
+            if a.get_figure() is not self:
+                msg = "The Axes must have been created in the present figure"
+                raise AxesException(msg)


I quite rarely construct my own exception types, the built-in exceptions are pretty comprehensive. In this case, a ValueError would be just as valuable IMHO (https://docs.python.org/2/library/exceptions.html#exceptions.ValueError). Thoughts?

fine with me.

montefra · 2014-05-20T14:49:41Z

I'll try to get back with further commits later this week or next one.

montefra · 2014-05-28T21:45:22Z

Before I embark myself into lib/matplotlib/backends (in the various modules in the directory there are 33 assert).
Are the modules in there supposed to be used by users (in which case most or all the assert should be changed) or should be considered as internals (in which case the assert are fine)?

montefra · 2014-05-28T21:47:11Z

ps: before anyone asks or complain. I'm going to run the test suite and adjust it if needed once I'm done with removing the asserts

tacaswell · 2014-05-29T02:45:03Z

I would avoid touching the backends just yet, they are on my list of things to re-factor as soon as 1.4 gets out (see #2624 ).

montefra · 2014-05-30T08:21:07Z

I've left alone a few asserts on private functions after checking that no user input is directly passed to them.
I've also left alone at least one in docstring.py assuming that are not used interactively or dynamically.

montefra · 2014-06-03T06:10:55Z

this line looks like a but to me. Shouldn't it be dt2 = r2.dtype[name]?

If it's a bug, I'll fix it here

Besides: I didn't dare to touch lib/matplotlib/mathtext.py

tacaswell · 2014-06-03T15:09:17Z

Please make a separate PR for that bug.

tacaswell · 2014-06-03T15:11:38Z

Also, I think that entire block of functions are candidates to be removed from the library. I have never looked at them before, but scanning them they look like they are replicating much of the functionality that pandas provides.

montefra · 2014-06-03T15:12:32Z

@tacaswell: I'll do a separate PR.

montefra · 2014-06-12T16:26:20Z

I'm waiting Travis answer in case I need to fix any problem.

And I have a question about testing.
Here it's written

run the script tests.py in the root directory of the distribution
On my system if I do this, it imports the installed version (as probably should be?)

If I cd build/lib.linux-x86_64-2.7 and try to run test.py, it loads the right matplotlib, but it fails at a certain point when it tries to use a mock (or mocks) module.
I guess that what's written above works if matplotlib is built with

python setup.py develop

It is correct?

montefra · 2014-06-18T19:36:35Z

I've installed matplotlib as develop and run python tests.py. After fixing some bug I've introduced (my bad), I get a large number of warnings and :

FAILED (KNOWNFAIL=365, errors=1, failures=3)

The failing tests are:

ERROR: matplotlib.tests.test_backend_pgf.test_rcupdate
with

[...]
File "/.../matplotlib/lib/matplotlib/backends/backend_pgf.py", line 318, in __init__
raise LatexError("LaTeX returned an error, probably missing font or error in preamble:\n%s" % stdout)
LatexError: LaTeX returned an error, probably missing font or error in preamble:
This is pdfTeX, Version 3.1415926-2.5-1.40.14 (TeX Live 2013/TeX Live for SUSE Linux)
restricted \write18 enabled.

FAIL: matplotlib.tests.test_coding_standards.test_pep8_conformance
I can fix it, but in a different PR

FAIL: matplotlib.tests.test_text.test_font_styles.test with

ImageComparisonFailure: images not close:    /.../matplotlib/result_images/test_text/font_styles.png vs. /.../matplotlib/result_images/test_text/font_styles-expected.png (RMS 13.672)

FAIL: matplotlib.tests.test_text.test_font_styles.test with

ImageComparisonFailure: images not close: /.../matplotlib/result_images/test_text/font_styles_pdf.png vs. /.../matplotlib/result_images/test_text/font_styles-expected_pdf.png (RMS 14.065)

Now I run python3 tests and update here. Then I'll push the corrected version.

montefra · 2014-06-18T20:07:48Z

With python3 I get three extra errors:

ERROR: Failure: AttributeError ('module' object has no attribute 'test_dates')
ERROR: Failure: AttributeError ('module' object has no attribute 'test_legend')
ERROR: Failure: AttributeError ('module' object has no attribute 'test_patheffects')

The traceback is always the same (except for the actual attribute name)

Traceback (most recent call last):
  File "/home/susefra/.local/lib/python3.3/site-packages/nose/failure.py", line 39, in runTest
    raise self.exc_val.with_traceback(self.tb)
  File "/home/susefra/.local/lib/python3.3/site-packages/nose/loader.py", line 403, in loadTestsFromName
    module = resolve_name(addr.module)
  File "/home/susefra/.local/lib/python3.3/site-packages/nose/util.py", line 321, in resolve_name
    obj = getattr(obj, part)
AttributeError: 'module' object has no attribute 'test_patheffects'

tacaswell · 2015-02-07T23:10:31Z

At a minimum this needs a re-base.

@pelson @mdboom @efiring Thoughts? I think I am in favor of switching asserts in public facing function into exceptions so we can provide more meaningful error messages/make sane error handling possible. I think private functions should keep using assert as they can in principle be suppressed for efficiency reasons.

mdboom · 2015-02-09T15:44:14Z

@tacaswell: I agree with that assert vs. exception policy. @montefra: Sorry this has sat here so long. It would be nice to rebase this so we can merge.

efiring · 2015-02-09T17:05:08Z

@tacaswell: Yes, I agree also.

montefra · 2015-02-10T00:10:17Z

At the moment I'm travelling, so I don't know when I'll do it. For sure I can rebase, test and commit within a week and a half. Is there any hurry?

tacaswell · 2015-03-16T02:02:30Z

lib/matplotlib/transforms.py

@@ -2436,8 +2463,8 @@ def __init__(self, boxin, boxout, **kwargs):
        Create a new :class:`BboxTransform` that linearly transforms
        points from *boxin* to *boxout*.
        """
-        assert boxin.is_bbox
-        assert boxout.is_bbox
+        if not boxin.is_bbox or not boxout.is_bbox:


or should be and

tacaswell · 2015-03-16T02:07:50Z

@montefra I left you a bunch of style-related comment and a few places where I was confused by the logic.

Thank you for doing this!

montefra · 2015-03-16T12:59:52Z

@tacaswell it seems that we now agree 👍

Should I rebase?

tacaswell · 2015-03-22T03:35:08Z

lib/matplotlib/table.py

@@ -482,12 +482,17 @@ def table(ax,
    rows = len(cellText)
    cols = len(cellText[0])
    for row in cellText:
-        assert len(row) == cols
+        if len(row) != cols:
+            msg = "Each row in 'cellText' must have {} columns"


This formatting does not work with python 2.6, you have to explicitly put in either index or kwargs.

tacaswell · 2015-03-22T03:43:04Z

This is getting really close! Just a few minor issues (one class of 2.6 compatibility one class of style consistency).

Thank you so much for working on this!

tacaswell · 2015-03-22T03:44:01Z

Please @ ping me when this is updated.

montefra · 2015-03-22T10:04:48Z

@tacaswell: on top of your comments, I went through all my modifications to make the whole thing more consistent.

ps: I've found a few more "{}".format(..) around mpl, so they should fail on python 2.6. I'll do a new PR to fix them.

tacaswell · 2015-03-22T18:35:29Z

Awesome.

A PR to clean up the format calls else where would be appreciated.

MNT : converted assert into exception Convert input-validation assertions to raise Exceptions (mostly ValueError) instead.

WeatherGod · 2015-05-23T13:20:44Z

lib/matplotlib/transforms.py

-        assert np.prod(angles.shape) == angles.shape[0] == pts.shape[0]
+        if angles.ndim!=1 or angles.shape[0] != pts.shape[0]:
+            msg = "'angles' must be a column vector and have same number of"
+            msg += " rows as 'pts'"


whoops! looks like we forgot to add a raise here. This is fixed in #4458

tacaswell added this to the v1.4.x milestone May 17, 2014

pelson reviewed May 20, 2014
View reviewed changes

montefra mentioned this pull request May 30, 2014

Various improvements in finance.py #3094

Closed

3 tasks

montefra mentioned this pull request Jun 3, 2014

bug fix: check the type of the 'key' of the two array 'r1' and 'r2' #3111

Merged

tacaswell modified the milestones: 1.5.0, v1.4.x Feb 7, 2015

tacaswell added status: needs review status: needs revision labels Feb 7, 2015

tacaswell reviewed Mar 16, 2015
View reviewed changes

montefra added 2 commits March 16, 2015 10:38

Modified according to @tacaswell comments

41bf6b5

fixed pep8 Travis failure

7ea5c1a

tacaswell mentioned this pull request Mar 19, 2015

Implemented issue 4044. Created a Cell subclass, SciCell, to override th... #4245

Closed

tacaswell reviewed Mar 22, 2015
View reviewed changes

tacaswell added status: needs revision and removed status: needs review labels Mar 22, 2015

python2.6 string formatting. style more uniform

d6c3c32

tacaswell added a commit that referenced this pull request Mar 22, 2015

Merge pull request #3060 from montefra/no_assert

8a270fc

MNT : converted assert into exception Convert input-validation assertions to raise Exceptions (mostly ValueError) instead.

tacaswell merged commit 8a270fc into matplotlib:master Mar 22, 2015

tacaswell removed the status: needs revision label Mar 22, 2015

montefra deleted the no_assert branch March 22, 2015 18:51

WeatherGod reviewed May 23, 2015
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

converted assert into exception #3060

converted assert into exception #3060

montefra commented May 13, 2014

montefra commented May 13, 2014

WeatherGod commented May 13, 2014

montefra commented May 13, 2014

WeatherGod commented May 13, 2014

tacaswell commented May 17, 2014

pelson May 20, 2014

montefra May 20, 2014

montefra commented May 20, 2014

montefra commented May 28, 2014

montefra commented May 28, 2014

tacaswell commented May 29, 2014

montefra commented May 30, 2014

montefra commented Jun 3, 2014

tacaswell commented Jun 3, 2014

tacaswell commented Jun 3, 2014

montefra commented Jun 3, 2014

montefra commented Jun 12, 2014

montefra commented Jun 18, 2014

montefra commented Jun 18, 2014

tacaswell commented Feb 7, 2015

mdboom commented Feb 9, 2015

efiring commented Feb 9, 2015

montefra commented Feb 10, 2015

tacaswell Mar 16, 2015

montefra Mar 16, 2015

tacaswell commented Mar 16, 2015

montefra commented Mar 16, 2015

tacaswell Mar 22, 2015

tacaswell commented Mar 22, 2015

tacaswell commented Mar 22, 2015

montefra commented Mar 22, 2015

tacaswell commented Mar 22, 2015

WeatherGod May 23, 2015

converted assert into exception #3060

converted assert into exception #3060

Conversation

montefra commented May 13, 2014

montefra commented May 13, 2014

WeatherGod commented May 13, 2014

montefra commented May 13, 2014

WeatherGod commented May 13, 2014

tacaswell commented May 17, 2014

pelson May 20, 2014

Choose a reason for hiding this comment

montefra May 20, 2014

Choose a reason for hiding this comment

montefra commented May 20, 2014

montefra commented May 28, 2014

montefra commented May 28, 2014

tacaswell commented May 29, 2014

montefra commented May 30, 2014

montefra commented Jun 3, 2014

tacaswell commented Jun 3, 2014

tacaswell commented Jun 3, 2014

montefra commented Jun 3, 2014

montefra commented Jun 12, 2014

montefra commented Jun 18, 2014

montefra commented Jun 18, 2014

tacaswell commented Feb 7, 2015

mdboom commented Feb 9, 2015

efiring commented Feb 9, 2015

montefra commented Feb 10, 2015

tacaswell Mar 16, 2015

Choose a reason for hiding this comment

montefra Mar 16, 2015

Choose a reason for hiding this comment

tacaswell commented Mar 16, 2015

montefra commented Mar 16, 2015

tacaswell Mar 22, 2015

Choose a reason for hiding this comment

tacaswell commented Mar 22, 2015

tacaswell commented Mar 22, 2015

montefra commented Mar 22, 2015

tacaswell commented Mar 22, 2015

WeatherGod May 23, 2015

Choose a reason for hiding this comment