fix for issue #997

[AmitAronovitch]

see discussion in #997

[ianthomas23]

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).

[WeatherGod]

+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.

[AmitAronovitch]

You are right about the unit test, and I and I will be happy to add, but it
might take a week or two until I find time to do it. I have no objection if
someone else volunteers to do it before that.

As for checking differences, I propose trying to make a huge grid (use very
small dx or very small dy, as much as memory would allow), and check x,y
and interpolated values at the edge of the "long" dimension. The patched
version uses accumulated addition as opposed to multiplication, so it might
have a different pattern of rounding errors. I can also do that when I add
the test. Let me know if you have more ideas.
On 20 ביול 2012 16:30, "Benjamin Root" <
reply@reply.github.com>
wrote:

+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.

---

Reply to this email directly or view it on GitHub:
#998 (comment)

[pelson]

@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.

Thanks,

[AmitAronovitch]

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?

[pelson]

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

[AmitAronovitch]

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).

[AmitAronovitch]

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

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

or

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

[pelson]

Personally, I would prefer it to be in test_delunay.py. 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.

Thanks,

[dmcdougall]

@pelson Agreed, it should go in test_delaunay.py.

[AmitAronovitch]

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).