Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.1k
Replace warnings.warn with cbook._warn_external or logging.warning#12006
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
Replace warnings.warn with cbook._warn_external or logging.warning#12006
Uh oh!
There was an error while loading.Please reload this page.
Conversation
jklymak commentedSep 2, 2018
Wow, looks great! You are going to have to rebase However, is there no way for us to automate this? Does it have to be set manually for every warnings call? I admit that a quick google search didn't turn up anything hopeful, but thought I'd ask... |
jklymak commentedSep 2, 2018
We also don't allow lines >79 characters, so you'll have to sort out all the overspilling lines (see the flake8 errors in travis-ci/py3.6 build) |
217e695 to3c71722Compareanntzer commentedSep 3, 2018
In general, this is an improvement. But an even better improvement would be to just use _warn_external everywhere -- see#11298 (and adapt the (nice) explanation for it). (See for example (not in this PR) backend_pdf's writeInfoDict which currently warns with stacklevel=2 on invalid keywords, but that'll point to some other internal method of backend_pdf which only makes things more (not less) confusing for the end user.) However, it would be even betternot to apply either stacklevel=2, or _warn_external, indiscriminately. For example, the change in backend_bases.py in this PR is in key_press_handler, which is invoked asynchronously by the GUI event loop (well, I don't know if there's a good value for stacklevel in that case, so perhaps we can just live with it...). |
hershen commentedSep 5, 2018
Oh, that's a nice idea in _warn_external. Sorry, I didn't follow your key_press_handler example. Why would _warn_external not work well in that case? And a git question - is there a way to continue working on this branch without polluting the history (for example changing warnings.warn(___, stacklevel=2) to _warn_external)? Or is it better to just create a new branch from master? |
anntzer commentedSep 5, 2018
Perhaps key_press_handler is not the best example but I remember looking into doing a global replace and thinking that some of the warn calls would become more confusing if using _warn_external. I don't have a case right now, someone just needs to do a careful review of the changes. |
hershen commentedSep 6, 2018
Ok, thanks. I'm away from a computer until the middle of next week. When I return, I can do a global replace, as I don't know the code well enough to identify when _warn_external isn't appropriate. A more knowledgeable reviewer can then hopefully identify the completed cases. Will that be helpful, or should I pick a different issue to work on? Thanks for the git tip. I'll play around with it. |
anntzer commentedSep 6, 2018
Sounds good. |
lib/matplotlib/font_manager.py Outdated
| json.dump(data,fh,cls=JSONEncoder,indent=2) | ||
| exceptOSErrorase: | ||
| warnings.warn('Could not save font_manager cache {}'.format(e)) | ||
| warnings.warn('Could not save font_manager cache {}'.format(e),stacklevel=2) |
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.
For example, I don't think that this warning would get clearer if using _warn_external (or even stacklevel=2).
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.
OK, I see what you mean.
Is it safe to assume that this warning will be "triggered" when importing pyplot? If so, then does it make sense to print the user line of code that imports pyplot i.e.import matplotlib.pyplot as plt?
I tried to manipulate _warn_external to do this without success. If this solution is acceptable, any ideas how to achieve that?
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.
Is it safe to assume that this warning will be "triggered" when importing pyplot?
Yes
If so, then does it make sense to print the user line of code that imports pyplot i.e. import matplotlib.pyplot as plt?
I wouldn't overthink it (there isn't anything really smart to do in this case IMO) and would just trigger a warning at this line; or perhaps is actually makes sense to use logging.warning instead of warnings.warn here in fact, now that I think of it...
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.
After further thought, I convinced myself that that should exactly be the criterion forwarnings vslogging: by showing the line of offending code, does that point the user to something they should modify?
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.
Sounds good, thanks.
timhoffm commentedOct 13, 2018
@hershen still interested to work on this? |
hershen commentedOct 13, 2018
I am. Sorry it got set aside for so long. I'll try to finish it this weekend. |
timhoffm commentedOct 13, 2018
No hurry. Just wanted to make sure this is not orphaned. |
aa8420c tob00921bComparehershen commentedOct 16, 2018
I (finally) switched the
|
| ax._set_position(newpos,which='original') | ||
| else: | ||
| warnings.warn('constrained_layout not applied. At least ' | ||
| cbook._warn_ext('constrained_layout not applied. At least ' |
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.
typo
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.
Good catch, thanks!
lib/matplotlib/artist.py Outdated
| """ | ||
| ifrasterizedandnothasattr(self.draw,"_supports_rasterization"): | ||
| warnings.warn("Rasterization of '%s' will be ignored"%self) | ||
| logging.warning("Rasterization of '%s' will be ignored"%self) |
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.
looks like cbook._warn_external
| defhandle_unknown_event(self,event): | ||
| warnings.warn('Unhandled message type {0}. {1}'.format( | ||
| event['type'],event),stacklevel=2) | ||
| cbook._warn_external('Unhandled message type {0}. {1}'.format( |
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 think this should use logging (it's basically an event handler and thus likely to be called asynchronously anyways).
| # looks like they forgot to set the image type drop | ||
| # down, going with the extension. | ||
| warnings.warn( | ||
| cbook._warn_external( |
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.
logging
(this is most likely called throgh gui interaction)
| # looks like they forgot to set the image type drop | ||
| # down, going with the extension. | ||
| warnings.warn( | ||
| cbook._warn_external( |
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 as above
lib/matplotlib/backend_bases.py Outdated
| ax.set_yscale('log') | ||
| exceptValueErrorasexc: | ||
| warnings.warn(str(exc)) | ||
| cbook._warn_external(str(exc)) |
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.
logging (most likely called through gui interaction)
lib/matplotlib/backend_bases.py Outdated
| ax.set_xscale('log') | ||
| exceptValueErrorasexc: | ||
| warnings.warn(str(exc)) | ||
| cbook._warn_external(str(exc)) |
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 as above
lib/matplotlib/lines.py Outdated
| # Convert pick radius from points to pixels | ||
| ifself.figureisNone: | ||
| warnings.warn('no figure set when check if mouse is on line') | ||
| logging.warning('no figure set when check if mouse is on line') |
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 think warn_external here (can be callback or direct call).
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.
Isn't this gui interaction?
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 was thinking that one can also call contains() directly, but actually that's almost certainly much rarer, so logging is fine.
lib/matplotlib/mathtext.py Outdated
| ifo==0: | ||
| iflen(self.children): | ||
| warnings.warn( | ||
| cbook._warn_external( |
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.
also logging? a bit borderline but consistent with the rest of the mathtext module
lib/matplotlib/__init__.py Outdated
| # if this fails, we will just write to stdout | ||
| exceptIOError: | ||
| warnings.warn('could not open log file "{0}"' | ||
| logging.warning('could not open log file "{0}"' |
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.
throughout the PR:
- You should use the module-level logger
_log = logging.getLogger(__name__)then_log.warning(...). - Alignment of subsequent lines are a bit messed up sometimes.
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.
- Ah, right. I even read that I should do that, but for some reason it didn't register.
- Thanks for pointing that out. I wasn't really paying attention to that.
anntzer commentedOct 17, 2018
Thanks for the thorough changes! Left some comments. |
416fed2 to1bece33Comparelib/matplotlib/mathtext.py Outdated
| ############################################################################## | ||
| # FONTS | ||
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.
no empty line here?
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 added it because I gotE302 expected 2 blank lines, found 1 from flake8 when it wasn't there.
Is there a better solution?
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.
Putting the blank linebefore the ##### seems more logical? Should hopefully also placate the style checker.
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.
You're right. That fixed it.
lib/matplotlib/tight_layout.py Outdated
| matplotlib.cbook._warn_external( | ||
| 'The left and right margins cannotbe made large enough to ' | ||
| 'accommodate all axes decorations. ') | ||
| matplotlib.cbook._warn_external('The left and right margins cannot ' |
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.
just dofrom matplotlib import cbook at the top?
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.
Fixed, thanks.
anntzer left a comment
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.
Only minor formatting points left.
6e355a7 tobd01a00Comparehershen commentedOct 19, 2018
Squashed. |
QuLogic left a comment
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.
A few typos; I mostly assume@anntzer has looked over the other bits.
doc/devel/contributing.rst Outdated
| `warnings.warn`) is that `cbook._warn_external` should be used for things the | ||
| user must change to stop the warning (typically in the source), whereas | ||
| `logging.warning` can be more persistent. Moreover, note that | ||
| `cbook._warn_extrenal` will by default only emit a given warning *once* for |
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.
Typo: extrenal
doc/devel/contributing.rst Outdated
| layouting or rendering) should only log at this level. | ||
| By default, `warnings.warn` displays the line of code that has the `warn` call. | ||
| This usually isn't more informative than the warning message itself. Therefore, | ||
| Matplotlib uses `cbook._warn_external` which uses `warnings.warn`, but it goes |
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.
Remove 'it'
doc/devel/contributing.rst Outdated
| up the stack and displays the first line of code outside of Matplotlib. | ||
| For example, for the module:: | ||
| #in my_matplotlib_module.py |
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.
Space after#
doc/devel/contributing.rst Outdated
| running the script:: | ||
| from matplotlib import my_matplotlib_module | ||
| my_matplotlib_module.set_range(0,0) #set range |
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.
Space after, and#; two spaces before#.
doc/devel/contributing.rst Outdated
| UserWarning: Attempting to set identical bottom==top | ||
| warnings.warn('Attempting to set identical bottom==top') | ||
| Modifiying the module to use `cbook._warn_extermal`:: |
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.
Typo: Modifiying; extermal.
lib/matplotlib/bezier.py Outdated
| """ | ||
| importwarnings | ||
| importmatplotlib.cbookascbook |
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.
Move down to below numpy (can also add a blank line after numpy)
lib/matplotlib/colorbar.py Outdated
| importlogging | ||
| importwarnings | ||
| importmatplotlib.cbookascbook |
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.
Move down to matplotlib section below.
| frommatplotlibimportcbook,rcParams,backend_tools | ||
| importwx | ||
| importlogging |
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.
Move up to stdlib section of imports.
lib/matplotlib/dates.py Outdated
| 'range. It may be necessary to add an ' | ||
| 'interval value to the ' | ||
| 'AutoDateLocator\'s intervald ' | ||
| 'dictionary. Defaulting to {' |
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.
Funny place to break; better to keep{0} together.
lib/matplotlib/image.py Outdated
| warnings.warn( | ||
| "Casting input data from '{0}' to 'float64'" | ||
| "forimshow".format(A.dtype)) | ||
| cbook._warn_external("Casting input data from '{" |
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 here; keep{0} together.
hershen commentedOct 21, 2018
Thanks for the thorough review@QuLogic. I addressed all your comments (and fixed a few other places where I had the same issues). I'll squash again when you're happy. |
hershen commentedOct 21, 2018
Sorry, but what did I do to make the travis/no language test angry? |
QuLogic commentedOct 21, 2018
Seems to be a bug in Homebrew's NumPy package. Feel free to squash, but it probably won't fix it. |
6cf17f3 to9ecdec3Comparehershen commentedOct 22, 2018
OK. Squashed. The travis issues here seem to be common to all the commits in PRs in the last ~15 hours, so I assume it's not something I'm did wrong. |
9ecdec3 to226f1d4Comparehershen commentedOct 27, 2018
Rebased. |
timhoffm left a comment
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.
Thanks for working through all the warnings.
| warnings.warn("streamed pgf-code does not support raster " | ||
| "graphics, consider using the pgf-to-pdf option", | ||
| UserWarning,stacklevel=2) | ||
| _log.warning("streamed pgf-code does not support raster " |
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.
Isn't this of the category "things the that the user must change to stop the warning" and thus should be_warn_external?
| defhandle_unknown_event(self,event): | ||
| warnings.warn('Unhandled message type {0}. {1}'.format( | ||
| event['type'],event),stacklevel=2) | ||
| _log.warning('Unhandled message type {0}. {1}'.format( |
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.
also_warn_external?
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 was changed to_log.warning following@anntzer's suggestion.
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.
In practice this will almost always get called asynchronously by the webserver event handler so "external" will point to the call to mainloop(), which is not really helpful.
| # looks like they forgot to set the image type drop | ||
| # down, going with the extension. | ||
| warnings.warn( | ||
| _log.warning( |
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.
improper use ->_warn_external?
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 was changed to_log.warning following@anntzer's suggestion.
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.
In practice this will almost always get called through a wx GUI event handler so "external" will point to the call to mainloop(), which is not really helpful.
| # looks like they forgot to set the image type drop | ||
| # down, going with the extension. | ||
| warnings.warn( | ||
| _log.warning( |
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.
improper use ->_warn_external?
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 was changed to_log.warning following@anntzer's suggestion.
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.
In practice this will almost always get called through a wx GUI event handler so "external" will point to the call to mainloop(), which is not really helpful.
| rgba=mcolors.to_rgba(color) | ||
| exceptValueError: | ||
| warnings.warn('Ignoring invalid color %r'%color,stacklevel=2) | ||
| _log.warning('Ignoring invalid color %r'%color) |
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.
improper value passed ->_warn_external?
lib/matplotlib/figure.py Outdated
| warnings.warn('Matplotlib is currently using %s, which is a ' | ||
| 'non-GUI backend, so cannot show thefigure.' | ||
| %get_backend()) | ||
| andwarn): |
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.
Why the additional spaces?PEP-8 example suggests:
if (this_is_one_thing and that_is_another_thing): do_something()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.
Good catch!
| "Using bbox_to_anchor=(0,0,1,1) now.") | ||
| cbook._warn_external("Using the axes or figure transform " | ||
| "requires a bounding box in the respective " | ||
| "coordinates. Using bbox_to_anchor=(0,0,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.
I wouldn't break the parenthesis for better readability in the code. Would break befrore "Using" here even though the line will get quite short.
lib/mpl_toolkits/mplot3d/axes3d.py Outdated
| self._cids= [c1,c2,c3] | ||
| else: | ||
| warnings.warn( | ||
| _log.warning( |
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.
Is this not also a case the user should do something about (and thus_warn_external).
| ifnothas_include_file( | ||
| ext.include_dirs,os.path.join("numpy","arrayobject.h")): | ||
| warnings.warn( | ||
| _log.warning( |
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.
User action required ->_warn_extenal.
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 doesn't live in the matplotlib package so "external" will point to this very file; showing the source won't help.
hershen commentedOct 28, 2018
anntzer commentedOct 29, 2018
Agreed with@timhoffm on some, left comments on others. |
…_external.2) Updated contributions guidelines to use cbook._warn_external.
226f1d4 to67e57b2Comparehershen commentedOct 29, 2018
Added@timhoffm's suggestions. |
jklymak commentedOct 29, 2018 • 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.
|
kbrose commentedNov 15, 2018
Can the title of this PR be updated to reflect its contents? I had a hard time finding it because its title ("Added stacklevel=2 to all warnings.warn calls (issue 10643)") leaves out the important part of changing every warning.warn call. |
PR Summary
Fix issue#10643
Note:
I liked@ImportanceOfBeingErnest 's suggestion of advising to set stacklevel in the documentation. I didn't find a good place for it, so I created a new section in contributing.rst. Please let me know if there's a better place or if it's too long.
PR Checklist