Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

fix rendering slowdown with big invisible lines (issue #1256) #1531

Merged
merged 1 commit into from Dec 6, 2012

Conversation

Projects
None yet
4 participants
Contributor

pierre-haessig commented Nov 24, 2012

tiny change in Line2D.draw to return immediately if the line is not visible.
This avoids annoying rendering slowdown with big Line2D objects (say lines holding about 10**7 points)

Contributor

pierre-haessig commented Nov 25, 2012

I just realized that when I introduced my testcase in __init__.py, I used an older version of this file by mistake. Basically, my commit a28c665 is screwed... I guess that amending is not a good practice. Maybe I should start a fresh branch !

@pelson pelson and 1 other commented on an outdated diff Nov 26, 2012

lib/matplotlib/lines.py
@@ -484,6 +484,9 @@ def _is_sorted(self, x):
@allow_rasterization
def draw(self, renderer):
+ "draw the Line with `renderer` unless visiblity is False"
@pelson

pelson Nov 26, 2012

Member

I know it is not consistent throughout mpl, but we are trying to standardise on the """ style docstring. Would you mind updating?

@pierre-haessig

pierre-haessig Dec 1, 2012

Contributor

Done! I usually use triple quotes too, but I wanted to follow some convention so I had just looked at a function a few lines above... which has single quotes ;-)

@pelson pelson and 1 other commented on an outdated diff Nov 26, 2012

lib/matplotlib/tests/README
@@ -0,0 +1,15 @@
+About creating a new test module in matplotlib.tests
@pelson

pelson Nov 26, 2012

Member

Hmmm. @mdboom has just updated the coding_guide page to explain this in a little more detail.

Do you feel that having this extra README for added help outweighs the cost of having to maintain this information in 2 places?
Personally, I'm not sure - hence would like to get your more fresh eyes on this.

@mdboom

mdboom Nov 26, 2012

Owner

Yes -- I don't think we should be duplicating this information, and the developer docs were recently reorganized to (hopefully) make this easier to find. @pierre-haessig : would you mind looking at the new developer docs (on matplotlib.org) and letting me know if you think things are more obvious?

@mdboom

mdboom Nov 26, 2012

Owner

BTW: I wouldn't oppose adding this README file if it contained only a link to the online documentation.

@pelson pelson and 1 other commented on an outdated diff Nov 26, 2012

lib/matplotlib/tests/test_lines.py
@@ -0,0 +1,52 @@
+"""
+Tests specific to the lines module.
+"""
+
+from nose.tools import assert_true
+from timeit import repeat
+import numpy as np
+import matplotlib as mpl
+import matplotlib.pyplot as plt
+
+
+def test_invisible_Line_rendering():
@pelson

pelson Nov 26, 2012

Member

This test worries me. The way you have implemented it is about the best that I can imagine for using actual timings (because they are relative rather than absolute).

Based on these concerns (and the ability to test that the functionality that you have fixed continues to remain fixed) it makes me wonder if we can improve the implementation of Artist. I've just posted a message on the matplotlib-devel mailing list on the subject of "Use of meta on Artist". It may be that we stick with your approach (which does fix the problem), or that we decide to change the implementation a little.

@pierre-haessig

pierre-haessig Dec 1, 2012

Contributor

I understand that actual timing is a bit crude so if there is a better solution, it would be good to update this test. In the mean time, it kind of does the job.

@pelson

pelson Dec 3, 2012

Member

Agreed. Lets stick with this until it causes any actual problems...

Member

pelson commented Nov 26, 2012

@pierre-haessig : Thanks for doing this. It is a very useful fix which, as is, provides a good improvement in performance for large, non visible lines. As you can see, I've posted a couple of comments, none of which are major, but I have raised a question on the mpl-devel mailinglist about a possible alternative approach to resolving (and testing) this issue. I think we should hang fire on merging this for a couple of days/week(s) until that discussion has happened, but if the decision is against my proposal, what you have implemented here is the correct way to go.

Thanks again for doing this,

Phil

Contributor

pierre-haessig commented Dec 1, 2012

@pelson : Thank you very much for your feedback.

I've updated the README to only redirects people to devel doc. To answer @mdboom question about the new docs, I found the testing section immensely useful, because first I was very frustrated of my test not being found (I'm not familiar with nose, but I thought there was some automatic test discovery feature... ).

Then I found the solution by looking into the docs and that's why I made this small copy pasting which I agree is a pretty bad practice...
I think that putting a link to the docs in this README file is useful though.

Contributor

pierre-haessig commented Dec 1, 2012

About Travis build failing, I don't know what to do. Something is going wrong in the matplotlib.tests.test_mathtext where image comparison are involved. From what I see, the baseline image test_mathtext/mathfont_cm_00.png is 525x75 pixels while the test run creates a 800x600 images that it tries to compare with baseline. Thus the ValueError about shape mismatch.

This being said, I have no clue why the test wants to create a 800x600 image, especially since figure size is set by figsize=(5.25, 0.75)... Any ideas ? :-)

Member

pelson commented Dec 3, 2012

Any ideas ? :-)

Yes. Whenever you use pyplot for testing, you must either declare it as an Image test, or tell it to clearup after itself, the appropriate docs can be found here (don't worry, these have only just been added, and previously the cleanup decorator was undocumented).

Hope that does it!

Contributor

pierre-haessig commented Dec 5, 2012

@pelson, thanks for the explanation about the cleanup decorator. Hopefully, commit aacddf1 will resolve the test failures.

I suggest to use the .. warning:: Sphinx directive in the documentation to emphasize the need for this decorator with a nice red box. What do you think ?

Contributor

pierre-haessig commented Dec 5, 2012

Thanks to @pelson's help, there is no more Travis build failure :-)

Now I see two remaining issues:

  • the commit history of this pull request not pretty clean. Maybe some rebase is needed, but I'm not a git guru so I don't know really what to do, especially since these commits are already pushed online.
  • my master branch is lagging behind the main mpl master. I don't know if that's a problem for the PR... again I'm not a git guru !
Member

dmcdougall commented Dec 5, 2012

@pierre-haessig You don't need to incorporate the latest changes from master. At present, your pull request will merge cleanly in its current state. However if you wish to tidy up your commit history, you will need to interactively rebase the last 5 commits with git rebase -i HEAD~5. If you don't feel comfortable doing this, I can rebase them for you and tidy up the commits.

Contributor

pierre-haessig commented Dec 5, 2012

@dmcdougall I dived back in Pro Git book and ran git rebase -i HEAD~5. Apparently it ran successfully an returned:

Successfully rebased and updated refs/heads/master.

However, I see no effect on my commit history (I still see the 5 separate commits). I suspect it may relate to the fact I'm not working on a topic branch but on my master branch (which prooves itself pretty annoying...).
Now that my git history is maybe screwed I may wait before trying to git pull.

If you have any further git tips, I'll be glad to play a bit more ;-)

Member

dmcdougall commented Dec 5, 2012

@pierre-haessig Sounds like when you ran it, an editor popped up and you didn't change anything. If you try it again, read the stuff at the bottom of the text file that pops up. You can change commit messages and squash commits and such like.

Let me know how you get on.

fix rendering slowdown of big Lines (issue #1256)
About the fix:

Line2D.draw method now *returns immediately*,
if the Line is invisible (get_visible() returns False).

In particular, the `self.recache` and `self._transform_path`
calls are short-circuited.

In addition to the bugfix:

* adds a test case for lines rendering speed (issue #1256)
* adds a README in `matplotlib.tests` to redirect people
  to the testing section of the development guide.
Contributor

pierre-haessig commented Dec 5, 2012

@dmcdougall, you're right, I didn't change anything when the text editor popped up because I thought the default proposition was fine... I was wrong.
I finally found proper explanations about git rebase -i in http://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits and now I think my rebase went through ! (cf. c94c730)

I pushed with the --force option as I expected to be forced too... and apparently github swallowed that push smoothly :-)

Member

dmcdougall commented Dec 5, 2012

@pierre-haessig I dig the verbose commit message. Thanks. I'll let Travis finish up the test just for sanity's sake.

@pelson It appears, judging by the feedback on the mailing list, that the use of Meta on Artist is not encouraged in its most general setting. Are you happy with the solution presented here?

Member

pelson commented Dec 6, 2012

Are you happy with the solution presented here?

Yep. Good work @pierre-haessig 👍

dmcdougall added a commit that referenced this pull request Dec 6, 2012

Merge pull request #1531 from pierre-haessig/master
fix rendering slowdown with big invisible lines (issue #1256)

@dmcdougall dmcdougall merged commit 81086b3 into matplotlib:master Dec 6, 2012

1 check passed

default The Travis build passed
Details
Contributor

pierre-haessig commented Dec 7, 2012

Thank you all for your kind help and thanks for merging so fast. With this tiny PR, I've learned a lot about git, mpl testing and mpl code.

Member

pelson commented Dec 7, 2012

With this tiny PR, I've learned a lot about git, mpl testing and mpl code.

Great to hear! We're always looking for contributors such as yourself and I hope this experience has encouraged you to get involved and submit further mpl pull requests 😄.

Cheers,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment