fix for issue #997 #998

wants to merge 4 commits into

5 participants


see discussion in #997

Matplotlib Developers member

I can confirm that this PR solves the issue, and I don't think there are any detrimental side-effects.

The PR should ideally include a unit test; something based on the one in the issue request would be fine. Amit, if you need some help putting the test within the mpl unit test framework, let me know (I recently had to do the same for other delaunay changes).

Matplotlib Developers member

+1 on the unit test. Also, could you provide some possible insight of when/where the results may differ from before? We might want to make some tests to examine those cases as well to evaluate the differences.

Matplotlib Developers member

@AmitAronovitch : Did you have any progress on this? It is not going to make 1.2.0, but it can still be merged onto master at any point.



My own tests (direct calls to the function) seem to return identical results, but it seems that the existing Delaunay tests (image diffs) fail. I did not find the time to look into that yet (I want to extract the input and output used in these tests, before conversion to colormap, and check them directly).

p.s. Should the changeset be rebased/merged to the 1.2.0 branch, or to master?

Matplotlib Developers member

Thanks for the update.

Should the changeset be rebased/merged to the 1.2.0 branch, or to master?

You don't need to do either at the moment as your branch would currently merge cleanly into master, but if you do need to update it you should rebase (rather than merge) against master. HTH

Amit Aronovitch test_delaunay: expanded bounding box, to avoid numerical instability
due to points that lie exactly on the boundary.

The fix added in c43a02e allows the delaunay tests to pass cleanly with the proposed cpp patch.
This was an edge case caused by numerical difference of size 6.7e-16.
A better solution would be to fix Triangulation.prep_extrapolator itself, to make it more robust (see below), instead of fixing the test. However, I thought it might be better to keep that fix separate from #997.

The linear_extrapolator inserts nan for points that fall outside of its valid domain. Points that fall exactly on the corners of the specified bounding box may lie on the boundary of that domain, hence they might be considered "outside" in case of tiny rounding errors. The fix avoids this situation by specifying a bounding box that strictly contains all points of the grid. A better fix would be to make the domain of the linear extrapolator strictly contain the bounding box (it currently intersects it at two points).


For adding a test for this specific issue, should I:

a) insert it in (reason: localize all delaunay-related tests)


b) create a new file, e.g. (reason: it is quite different than the existing tests, and does not fit in their framework).

Matplotlib Developers member

Personally, I would prefer it to be in I haven't looked at that test module, but the name suggests to me that it should be a test of everything related to delunay triangulation - so feel free to pop it in there, and if someone would prefer it elsewhere we can adjust later on.


Matplotlib Developers member

@pelson Agreed, it should go in


Added the test, and also published an alternate fix: PR #1477.
I still prefer this one, because it avoids special-casing (and possibly there might be a tiny performance difference), but #1477 does not touch the loop code, so does not require further testing (see 3 below).
Both fixes currently work and pass tests.

I did not declare freetype versions in my test, as the other delaunay tests do. I only tested with 2.4.10, and I am not familiar with the possible font issues. Please advise if this might break something.

Remaining TODO:
1. Possibly handle freetype-version testing issues.
2. fix the problem discovered above in linear extrapolator code (this would make c43a02e redundant, but I think this fix might also avoid future problems).
3. test performance and accuracy differences in extreme cases (very large grid).

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