Gracefully handle non-finite z in tricontour (issue #10167) #14040
Conversation
There was a problem hiding this comment.
Does it make sense to just check the raw data so the expensive triangulation step is not performed first? I understand your point that a masked point may not end up in the triangulation, but that seems an edge case.
Second, I'd have thought NaN/masked values would just not get included in the algorithm rather than throwing an error.
Third, why are you calling min/max on data that isn't in the triangulation?
|
Does it make sense to just check the raw data so the expensive
triangulation step is not performed first? I understand your point that a
masked point may not end up in the triangulation, but that seems an edge
case.
The triangulation is already calculated in lines 80-81 (if it wasn't
user-specified). We could check all of the z array and avoid the np.unique
call which would be fine for the 99% of standard usage, but I'd prefer to
avoid obscure edge case bug reports in n years time.
Second, I'd have thought NaN/masked values would just not get included in
the algorithm rather than throwing an error.
It would be nice if that was the case, but it isn't. Some properties of the
triangulation are pre-calculated and cached for all subsequent
tricontour/tricontourf calls. If we allow the triangulation to be changed
e.g. via masked z values for each tricontour/tricontourf call, that caching
cannot occur and has to be recalculated each time. This is a big
performance impact for people who are using tricontour/tricontourf
sensibly. Without the caching the tricontour C++ is very sub-optimal and
would have to be replaced by a completely different algorithm.
Third, why are you calling min/max on data that isn't in the triangulation?
To keep the code the same as in contour.py. Every time I visit
tricontour.py I discover that there have been many modifications to the
comparable code in contour.py without any consideration of tricontour.py,
and I have to try to bring it back in line. But in this case you are right,
I should be using z_check instead. I will do so shortly.
|
|
|
||
| with pytest.raises(ValueError) as excinfo: | ||
| plt.tricontourf(triang, [0, 1, 2, np.inf]) | ||
| excinfo.match(r'z array cannot contain non-finite values') |
There was a problem hiding this comment.
You can also pass the match string as a parameter to pytest.raises.
Will keep that in mind in future reviews, sorry about that. |
| triang = mtri.Triangulation(x, y) | ||
| plt.figure() | ||
|
|
||
| with pytest.raises(ValueError) as excinfo: |
There was a problem hiding this comment.
To elaborate on the below comment, this can just be
with pytest.raises(ValueError, match='z array cannot contain non-finite values'):and the match line below can be removed
lib/matplotlib/tri/tricontour.py
Outdated
| # in the triangulation. | ||
| z_check = z[np.unique(tri.get_masked_triangles())] | ||
| if np.ma.is_masked(z_check): | ||
| raise ValueError('z cannot be a masked array') |
There was a problem hiding this comment.
This might be misleading. Maybe, "z must not contain masked points within the triangulation".
lib/matplotlib/tri/tricontour.py
Outdated
| if np.ma.is_masked(z_check): | ||
| raise ValueError('z cannot be a masked array') | ||
| if not np.isfinite(z_check).all(): | ||
| raise ValueError('z array cannot contain non-finite values') |
There was a problem hiding this comment.
Similarly, maybe "z array must not contain non-finite values within the triangulation".
|
@ianthomas23 I would like to get this merged within a very few days so that it will be included in the release that @tacaswell is planning. Would you like to make the small error message changes I suggested? Or do you prefer that I do it? Or that they be left as they are? |
|
@efiring I am happy to make your proposed changes and was intending to do so on Tuesday. If you would like to do so yourself before then that is fine by me. I think there will also need to be corresponding textual changes in the test code. |
|
Thank you, Tuesday will be fine. |
|
@efiring I've changed the error messages. |
|
Please placate flake8: |
|
@meeseeksdev backport to v3.1.x |
tricontourandtricontourfexpect azarray containing finite values, so nonp.nan,np.infor masked values. Up until now this has not been checked and a non-finite value causes a segmentation fault in the underlying C++ code.This bug was reported in issue #10167.
The fix is to explicitly check for non-finite values and raise an exception. The only complexity is that a triangulation may not include all the specified points and so we only need to check those points that are included in the triangulation.
Fix includes tests for
np.inf,-np.inf,np.nanandnp.mamasked values.