FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Custom error message for draw_path. issues : #8131 (bad error message from pyplot.plot) by herilalaina · Pull Request #8176 · matplotlib/matplotlib · GitHub

Repository navigation

Custom error message for draw_path. issues : #8131 (bad error message from pyplot.plot) - #8176

Merged
phobson merged 5 commits into
matplotlib:masterfrom
herilalaina:error-msg-plot
Apr 30, 2017
Merged

phobson merged 5 commits into
matplotlib:masterfrom
herilalaina:error-msg-plot

Conversation

Copy link
Copy Markdown
Contributor

Addressing the issue #8131, this PR adds reference to the agg.path.chunksize params in the overflow error message.

NelleV commented Mar 8, 2017

Copy link
Copy Markdown
Member

Hi @herilalaina
Sorry it took so long for us to review the pull request and thanks for the patch!

I'd rather have the try except surround only the part of the code that can raise an overflow error for readabiblity. It means having two try except, but I think we can live with that.
Do you mind making this small change?

Copy link
Copy Markdown
Contributor Author

Hi @NelleV, no problem. I will make this change as soon as I can :)

tacaswell added this to the 2.1 (next point release) milestone Mar 8, 2017

QuLogic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Typo needs to be fixed; tests are broken.

Comment thread lib/matplotlib/backends/backend_agg.py Outdated
else:
self._renderer.draw_path(gc, path, transform, rgbFace)
try:
self._renderer.draw_path(gc, p, transform, rgbFace)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Should be path, not p.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Sorry about that. thank you

Comment thread lib/matplotlib/backends/backend_agg.py Outdated
try:
self._renderer.draw_path(gc, p, transform, rgbFace)
except OverflowError as e:
e.args = ("Exceeded cell block limit (set 'agg.path.chunksize' rcparam)",)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'm not sure about this; should we raise a new exception?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I am not sure I understand your point. In fact, the same overflow error occurs if we set agg.path.chunksize with a big number. Here, I just change the overflow error message

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I mean, is changing e.args a good thing to do, vs raising a new exception entirely.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

in fact, if we raise new overflow error. We get something like this

Exception in Tkinter callback
Traceback (most recent call last):
File "/home/herilalaina/Documents/contribution/matplotlib_contrib/matplotlib/lib/matplotlib/backends/backend_agg.py", line 171, in draw_path
self._renderer.draw_path(gc, path, transform, rgbFace)
OverflowError: In draw_path: Exceeded cell block limit

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
File "/home/herilalaina/Documents/contribution/matplotlib_contrib/matplotlib_env/lib/python3.5/tkinter/init.py", line 1562, in call
return self.func(*args)
File "/home/herilalaina/Documents/contribution/matplotlib_contrib/matplotlib/lib/matplotlib/backends/backend_tkagg.py", line 284, in resize
self.show()
File "/home/herilalaina/Documents/contribution/matplotlib_contrib/matplotlib/lib/matplotlib/backends/backend_tkagg.py", line 355, in draw
FigureCanvasAgg.draw(self)
File "/home/herilalaina/Documents/contribution/matplotlib_contrib/matplotlib/lib/matplotlib/backends/backend_agg.py", line 475, in draw
self.figure.draw(self.renderer)
File "/home/herilalaina/Documents/contribution/matplotlib_contrib/matplotlib/lib/matplotlib/artist.py", line 68, in draw_wrapper
return draw(artist, renderer, *args, **kwargs)
File "/home/herilalaina/Documents/contribution/matplotlib_contrib/matplotlib/lib/matplotlib/figure.py", line 1240, in draw
renderer, self, artists, self.suppressComposite)
File "/home/herilalaina/Documents/contribution/matplotlib_contrib/matplotlib/lib/matplotlib/image.py", line 139, in _draw_list_compositing_images
a.draw(renderer)
File "/home/herilalaina/Documents/contribution/matplotlib_contrib/matplotlib/lib/matplotlib/artist.py", line 68, in draw_wrapper
return draw(artist, renderer, *args, **kwargs)
File "/home/herilalaina/Documents/contribution/matplotlib_contrib/matplotlib/lib/matplotlib/axes/_base.py", line 2386, in draw
mimage._draw_list_compositing_images(renderer, self, artists)
File "/home/herilalaina/Documents/contribution/matplotlib_contrib/matplotlib/lib/matplotlib/image.py", line 139, in _draw_list_compositing_images
a.draw(renderer)
File "/home/herilalaina/Documents/contribution/matplotlib_contrib/matplotlib/lib/matplotlib/artist.py", line 68, in draw_wrapper
return draw(artist, renderer, *args, **kwargs)
File "/home/herilalaina/Documents/contribution/matplotlib_contrib/matplotlib/lib/matplotlib/lines.py", line 800, in draw
line_func(renderer, gc, tpath, affine.frozen())
File "/home/herilalaina/Documents/contribution/matplotlib_contrib/matplotlib/lib/matplotlib/lines.py", line 1248, in _draw_solid
renderer.draw_path(gc, path, trans)
File "/home/herilalaina/Documents/contribution/matplotlib_contrib/matplotlib/lib/matplotlib/backends/backend_agg.py", line 173, in draw_path
raise OverflowError("Exceeded cell block limit (set 'agg.path.chunksize' rcparam)")
OverflowError: Exceeded cell block limit (set 'agg.path.chunksize' rcparam)

I will make the change if It's okay.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Obviously, it does appear to work, but I don't have an answer whether it is a good or idiomatic thing to be doing or not.

Opinions, @tacaswell?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Hi @QuLogic , checks failed on coverage. I'm new to code coverage. Do I need to implement test (for overflow error) to fix that ?

herilalaina force-pushed the error-msg-plot branch 3 times, most recently from eec2383 to 50dbb8f Compare March 27, 2017 08:55
nmax = rcParams['agg.path.chunksize'] # here at least for testing
npts = path.vertices.shape[0]

if (nmax > 100 and npts > nmax and path.should_simplify and

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This half of the if/else block is untested. Think you could add a test?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

hi @phobson , thank you. I will do it

Copy link
Copy Markdown
Member

👍 on this. To test the missed branch I assume you need to user a very long line with a chunk size that is too big?

herilalaina force-pushed the error-msg-plot branch 15 times, most recently from 55abee1 to df10f02 Compare April 4, 2017 16:12
Comment thread lib/matplotlib/tests/test_agg.py Outdated
rcParams['agg.path.chunksize'] = 8000000
with pytest.raises(OverflowError):
ax.plot(x, np.sin(x))
plt.show()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Hi @tacaswell , it doesn't raise OverflowError. Do you have any idea how to handle it?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is that chunk size big enough to cause a overflow?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It never raises overflow even if I increase chunk size. The test just crashed

Comment thread lib/matplotlib/tests/test_agg.py Outdated
ax = fig.add_subplot(1, 1, 1)

# Test with small chunksize
tmp = rcParams['agg.path.chunksize']

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

You don't need to worry about this; the cleanup on the test will reset all rcParams.

Comment thread lib/matplotlib/tests/test_agg.py Outdated
def test_chunksize():
x = range(9000000)
fig = plt.figure()
ax = fig.add_subplot(1, 1, 1)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

fig, ax = plt.subplots()

QuLogic commented Apr 12, 2017

Copy link
Copy Markdown
Member

I think the test is not doing anything because of the plt.show; replacing it with fig.canvas.draw() at least triggers something, though it still doesn't seem to cause an exception...

QuLogic commented Apr 12, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Actually, if all you want to do is test that side of the branch, you only need a path longer than the chunksize (which must be greater than 100), so set the chunksize to 105 and make a path that's 110 long. 9000000 is much too long, I think.

If you want to trigger the exception, that might be a little more difficult and I'm not sure how to do that.

QuLogic commented Apr 13, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

After some testing, it looks like you need roughly range(200000) to trigger the exception with chunksize<=100. That means that you'd need approximately range(20000000) to trigger an exception with chunksize>100. I think this is basically impossible so I don't think we should try to trigger the exception in the latter case.

Copy link
Copy Markdown
Contributor Author

Thank you for your great feedback ! I will fix them.

phobson merged commit 1ef4d39 into matplotlib:master Apr 30, 2017
herilalaina deleted the error-msg-plot branch June 8, 2017 14:06
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants


Back | FazBrowse Home | New Git URL