Skip to content

FIX: Contour lines rendered incorrectly when closed loops#19623

Merged
QuLogic merged 6 commits intomatplotlib:masterfrom
ianthomas23:19568_closed_contour_lines
Apr 8, 2021
Merged

FIX: Contour lines rendered incorrectly when closed loops#19623
QuLogic merged 6 commits intomatplotlib:masterfrom
ianthomas23:19568_closed_contour_lines

Conversation

@ianthomas23
Copy link
Copy Markdown
Member

PR Summary

Fixes issue #19568 so that closed contour line loops are rendered correctly.

Solution is to use PathCollections rather than LineCollections, the same as for filled contours. Added a specific test for this, and updated the 11 test images affected by the change.

To test the performance impact I've used the following code:

import matplotlib
matplotlib.use('Agg')
import matplotlib.pyplot as plt
import numpy as np

shape = (500, 1000)
x, y = np.meshgrid(np.linspace(0.0, 1.0, shape[1]), np.linspace(0.0, 1.0, shape[0]))

np.random.seed(9876)
z = np.random.uniform(size=shape)
z = np.ma.array(z, mask=np.random.uniform(size=shape) < 0.1)

fig = plt.figure(figsize=(80, 40))
ax = fig.add_axes([0.0, 0.0, 1.0, 1.0])
ax.contour(x, y, z)
fig.savefig('contour_lines.png')

which creates an 8000x4000 PNG with 117,000 line loops and 201,000 line strips. I timed it before and after this PR, 10 times each (yes I should have used timeit but I didn't) and the walltimes are:

  • Before PR: minimum = 11.994 s, mean = 12.081 s, stddev = 0.046 s
  • After PR: minimum = 12.273 s, mean = 12.301 s, stddev = 0.041 s

So minimum run time is 2.3% slower, mean run time is 1.8% slower. Let's call it 2% slower (on my new but cheap laptop). It is slower for 2 reasons:

  1. It is rendering more pixels.
  2. Time taken to determine if a line segment is closed or not, which here is performed ~318k times.

Item 2 could be made faster by moving the calculations from Python to C++. I would prefer to not to that now and instead consider it as part of a larger study into improving contouring performance.

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

@QuLogic
Copy link
Copy Markdown
Member

QuLogic commented Mar 3, 2021

As you mentioned in the issue, the C++ code creates the codes, and it seems like those are returned in both cases:

self.allsegs, self.allkinds = self._get_allsegs_and_allkinds()

Can you not use that in the _make_paths call?

@QuLogic
Copy link
Copy Markdown
Member

QuLogic commented Mar 3, 2021

Ah no, I see it doesn't:

def _get_allsegs_and_allkinds(self):
"""Compute ``allsegs`` and ``allkinds`` using C extension."""
allsegs = []
if self.filled:
lowers, uppers = self._get_lowers_and_uppers()
allkinds = []
for level, level_upper in zip(lowers, uppers):
vertices, kinds = \
self._contour_generator.create_filled_contour(
level, level_upper)
allsegs.append(vertices)
allkinds.append(kinds)
else:
allkinds = None
for level in self.levels:
vertices = self._contour_generator.create_contour(level)
allsegs.append(vertices)
return allsegs, allkinds

But wouldn't it be less trouble to go to that directly?

@ianthomas23
Copy link
Copy Markdown
Member Author

But wouldn't it be less trouble to go to that directly?

Not really. Returning the kind codes from C++ would save us a few lines of Python but make the C++ more complicated, and it needs to be in both _contour.cpp and _tri.cpp. Those C++ changes will be obsolete in a few months when I address the performance issues in a coherent way.

[Aside: The major performance issue for contour lines in the example here is that Python/C++ create 318k numpy arrays of vertices and 318k numpy arrays of kind codes which are passed to the renderer for consumption. That is a lot of memory thrashing. It is only done this way because it was necessary for the 'new' (2014) contour algorithm to match the 'old' (2005) one. After 7 years I think we can be brave enough to change this to, for example, a smaller number of larger numpy arrays. But that needs to be done carefully and is outside the scope of this PR.]

As you have asked, I have added a commit that returns the kind codes from the C++. Interestingly it is marginally slower than the Python-only solution but my timings are not comprehensive. I'd be happy to roll back the PR to the Python-only changes if that is preferred. It is certainly simpler and lower risk.

@ianthomas23 ianthomas23 force-pushed the 19568_closed_contour_lines branch from 9e14863 to 178012d Compare March 10, 2021 12:19
@ianthomas23 ianthomas23 force-pushed the 19568_closed_contour_lines branch from 3d6bfb9 to f8b6a5c Compare March 10, 2021 17:54
@ianthomas23
Copy link
Copy Markdown
Member Author

Tests are passing now that I have modified the examples and mplot3d in line with the other changes. I've also added some new tests to increase the coverage.

There are two possible solutions here:

  1. Simple Python-only fix (commit 1b15a11),
  2. More complicated but more correct C++ fix (latest commit).

Either would be acceptable in terms of fixing the problem. I slightly favour of the C++ approach but could be persuaded either way. It is riskier than the Python-only solution in that there will almost certainly be some glitches caused by it, but I have the time and motivation to deal with these so now seems as good a time as any.

@jklymak
Copy link
Copy Markdown
Member

jklymak commented Mar 22, 2021

I guess I don't understand the tradeoffs in putting the work in the C++ code. Certainly there are a lot of changes, but they seem to be mostly just adding codes to the structure? The python changes are simpler, so what is the goal of pushing it down to C++?

@tacaswell tacaswell added this to the v3.5.0 milestone Mar 22, 2021
@ianthomas23
Copy link
Copy Markdown
Member Author

The two implementations (Python and C++) are functionally equivalent and produce identical output. The difference is where the codes are calculated.

The benefit of the C++ implementation is that it is faster at determining if each contour line is a closed line loop or open line strip. However, this performance benefit is not a practical benefit as the time taken to create lots of numpy arrays swamps everything else. When I eventually improve the performance of contouring this will become important, but it isn't yet.

Copy link
Copy Markdown
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

This seems fine, and the tests look good. I wonder if this is an opportunity to ask for a few more comments, particularly in the C++ code as a guide to the uninitiated. These don't have to be voluminous, but a few more guideposts as to what is going on would be helpful?

@ianthomas23
Copy link
Copy Markdown
Member Author

The git diff isn't hugely helpful here; looking at the changed C++ is probably better as the relationship between the vertices and codes arrays is more obvious. But yes, I am happy to add more comments.

@ianthomas23
Copy link
Copy Markdown
Member Author

@jklymak I've added more comments to _contour.cpp and _tri.cpp to explain the data returned from C++ to python, and also in the python function _make_paths.

Copy link
Copy Markdown
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

I'm going to approve, because it looks complete, there are good amount of comments, and there are new tests. I'm not at all claiming I followed all of it ;-)

@QuLogic QuLogic merged commit d1c7743 into matplotlib:master Apr 8, 2021
@ianthomas23 ianthomas23 deleted the 19568_closed_contour_lines branch July 8, 2021 18:22
QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Sep 28, 2021
Also, update documentation for Attributes.

Followup to matplotlib#19623; fixes matplotlib#20906.
QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Sep 29, 2021
Also, update documentation for Attributes.

Followup to matplotlib#19623; fixes matplotlib#20906.
QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Sep 29, 2021
Also, update documentation for Attributes.

Followup to matplotlib#19623; fixes matplotlib#20906.
tacaswell pushed a commit to tacaswell/matplotlib that referenced this pull request Oct 12, 2021
Also, update documentation for Attributes.

Followup to matplotlib#19623; fixes matplotlib#20906.
tacaswell pushed a commit that referenced this pull request Oct 20, 2021
Also, update documentation for Attributes.

Followup to #19623; fixes #20906.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants