Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

converted assert into exception#3060

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

Merged
tacaswell merged 36 commits intomatplotlib:masterfrommontefra:no_assert
Mar 22, 2015
Merged

Conversation

montefra
Copy link
Contributor

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

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

I stumbled into anAssertionError without any explanation (I had to get the doc ofadd_axes).
My understanding is thatassert 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 thatassert 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
Copy link
ContributorAuthor

indef draw_artist(self, a) there is an otherassert: isa going to be available only afterdraw has been draw, or the user could call the method beforedraw? If the case is the latter, then I'll move it to exception.

@WeatherGod
Copy link
Member

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
Copy link
ContributorAuthor

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
Copy link
Member

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

Opinions on assertions vary, because they can be a statement ofconfidence about the correctness of the code. If you're certain that thecode is correct, then assertions are pointless, since they will neverfail and you can safely remove them. If you're certain the checks canfail (e.g. when testing input data provided by the user), then you darenot use assert since it may be compiled away and then your checks will beskipped.It's the situations in between those two that are interesting, times whenyou're certain the code is correct but not *quite* absolutely certain.Perhaps you've missed some odd corner case (we're all only human). Inthis case an extra runtime check helps reassure you that any errors willbe 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
.

@tacaswelltacaswell added this to thev1.4.x milestoneMay 17, 2014
@tacaswell
Copy link
Member

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

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)
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

fine with me.

@montefra
Copy link
ContributorAuthor

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

@montefra
Copy link
ContributorAuthor

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

@montefra
Copy link
ContributorAuthor

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
Copy link
Member

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
Copy link
ContributorAuthor

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 indocstring.py assuming that are not used interactively or dynamically.

@monteframontefra mentioned this pull requestMay 30, 2014
3 tasks
@montefra
Copy link
ContributorAuthor

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

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

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

@tacaswell
Copy link
Member

Please make a separate PR for that bug.

@tacaswell
Copy link
Member

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 thatpandas provides.

@montefra
Copy link
ContributorAuthor

@tacaswell: I'll do a separate PR.

@montefra
Copy link
ContributorAuthor

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 Icd build/lib.linux-x86_64-2.7 and try to runtest.py, it loads the rightmatplotlib, but it fails at a certain point when it tries to use amock (ormocks) module.
I guess that what's written above works if matplotlib is built with

python setup.py develop

It is correct?

@montefra
Copy link
ContributorAuthor

I've installed matplotlib asdevelop and runpython 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 runpython3 tests and update here. Then I'll push the corrected version.

@montefra
Copy link
ContributorAuthor

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
Copy link
Member

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
Copy link
Member

@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
Copy link
Member

@tacaswell: Yes, I agree also.

@montefra
Copy link
ContributorAuthor

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?

@@ -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:
Copy link
Member

Choose a reason for hiding this comment

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

or should beand

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

see above

@tacaswell
Copy link
Member

@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
Copy link
ContributorAuthor

@tacaswell it seems that we now agree 👍

Should I rebase?

@@ -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"
Copy link
Member

Choose a reason for hiding this comment

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

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

@tacaswell
Copy link
Member

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
Copy link
Member

Please @ ping me when this is updated.

@montefra
Copy link
ContributorAuthor

@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
Copy link
Member

Awesome.

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

tacaswell added a commit that referenced this pull requestMar 22, 2015
MNT : converted assert into exceptionConvert input-validation assertions to raise Exceptions (mostly ValueError) instead.
@tacaswelltacaswell merged commit8a270fc intomatplotlib:masterMar 22, 2015
@monteframontefra deleted the no_assert branchMarch 22, 2015 18:51
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'"
Copy link
Member

Choose a reason for hiding this comment

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

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

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.5.0
Development

Successfully merging this pull request may close these issues.

6 participants
@montefra@WeatherGod@tacaswell@mdboom@efiring@pelson

[8]ページ先頭

©2009-2025 Movatter.jp