Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

alternate fix for issue #997 #1477

Merged
merged 6 commits into from

5 participants

@AmitAronovitch

The fix proposed in #998 changes the looping method, and introduces (tiny) differences in accuracy. As opposed, this PR keeps the loop intact, and fixes the issue using a special-purpose if statement.

@AmitAronovitch AmitAronovitch referenced this pull request
Closed

fix for issue #997 #998

lib/matplotlib/tests/test_delaunay.py
@@ -186,4 +186,27 @@ def reference_test():
for func in allfuncs:
globals()['test_%s' % func.func_name] = make_test(func)
-make_all_testfuncs()
+make_all_2d_testfuncs()
+
+# 1d and 0d grid tests
+
+ref_interpolator = Triangulation([0,10,10,0],
+ [0,0,10,10]).linear_interpolator([1,10,5,2.0])
+
+def equal_arrays(a1,a2, tolerance=1e-10):
@pelson Collaborator
pelson added a note

Were you aware of numpy.testing? In particular the numpy.testing.assert_array_almost_equal function.

Nope, but always happy to learn.

help(assert_array_almost_equal) referred me to np.testing.assert_allclose, which in turn referenced np.allclose(). A useful np shortcut I was not aware of. Thanks :+1:
(p.s. It seems that generalizations added by assert_allclose, like handling inf/nan values, are redundant in this case, and probably not worth the complication of extra import and runtime, so I use plain allclose() ).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pelson pelson commented on the diff
lib/matplotlib/tests/test_delaunay.py
@@ -186,4 +186,27 @@ def reference_test():
for func in allfuncs:
globals()['test_%s' % func.func_name] = make_test(func)
-make_all_testfuncs()
+make_all_2d_testfuncs()
+
+# 1d and 0d grid tests
+
+ref_interpolator = Triangulation([0,10,10,0],
@pelson Collaborator
pelson added a note

Rather than using the module level namespace, it might be worth considering constructing a class which has "test_" methods.

When adding tests to an existing module, I should try staying close to existing style.
make_all_testfuncs() was there (I just renamed it), and since it used module functions rather than class methods, so did I
( and that would have been my default choice anyways in this case).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/matplotlib/tests/test_delaunay.py
((8 lines not shown))
+
+ref_interpolator = Triangulation([0,10,10,0],
+ [0,0,10,10]).linear_interpolator([1,10,5,2.0])
+
+def equal_arrays(a1,a2, tolerance=1e-10):
+ return np.all(np.absolute(a1 - a2) < tolerance)
+
+def test_1d_grid():
+ res = ref_interpolator[3:6:2j,1:1:1j]
+ assert equal_arrays(res, [[1.6],[1.9]])
+
+def test_0d_grid():
+ res = ref_interpolator[3:3:1j,1:1:1j]
+ assert equal_arrays(res, [[1.6]])
+
+@image_comparison(baseline_images=['delaunay-1d-interp'], extensions=['png'])
@pelson Collaborator
pelson added a note

I think we should probably remove the fonts from this plot. (see other examples in test_axes)

I thought the same myself, but wanted to get feedback, since these "freetype_version=" arguments seem to appear everywhere. Indeed, test_axes has a couple of tick-less plots. Point taken.

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

From a naive point of view, I prefer this change over #998. @ianthomas23 & others, would you mind reviewing and commenting whether this PR is preferable (if so, please close the old PR)?

Good work @AmitAronovitch !

Cheers,

lib/matplotlib/delaunay/_delaunay.cpp
@@ -289,8 +289,8 @@ static PyObject *linear_interpolate_grid(double x0, double x1, int xsteps,
if (!z) return NULL;
z_ptr = (double*)PyArray_DATA(z);
- dx = (x1 - x0) / (xsteps-1);
- dy = (y1 - y0) / (ysteps-1);
+ dx = ( x1==x0 ? 0 : (x1 - x0) / (xsteps-1) );
+ dy = ( y1==y0 ? 0 : (y1 - y0) / (ysteps-1) );
@WeatherGod Collaborator

Perhaps I am missing something here, but if x1==x0, then wouldn't the operation return zero anyway (unless xsteps==1)? If all we are doing is preventing division by zero errors, then wouldn't we rather want to test for xsteps==1?

@ianthomas23 Collaborator

I agree with @WeatherGod here, xsteps==1 (and similar for y) is better here.

@WeatherGod Collaborator

Also, as a bit of sanity check, is it ever possible for x/ysteps to be zero?

Argument validation for the grid specification arguments could be done on the python side. Thats basically in delaunay.interpolate.LinearInterpolator.getitem (but we should grep for other possible references).
Currently no validation is done. If we want to add them, we should think about the possible semantics of various cases.

To @WeatherGod (2): 0 for x/ysteps would just return an empty grid. If you put a negative value, it would fail before reaching the loop, because numpy raises an exception if you try to allocate an array with negative dimenson.

To @WeatherGod (1), @ianthomas23 : To be exact, I was checking for 0/0 (which is nan) rather than the general */0 (which would be "inf" unless the * is also 0).

The case with xsteps==1 and x0!=x1 may have valid use-cases, but it is not clear where the single point should be. Setting dx=0 in this case is equivalent to putting it on x0. We could, for example place it at 0.5*(x0+x1), or at x1. The stable code would fill the array with inf, which may be considered as some kind of error-indication.
On the other hand, I had actually used the case (x0==x1, xsteps>1) before, and from my point of view I was merely "extending its range of validity" to xsteps==0, without affecting the case described above.

However: I tend to think that having the "*/0" case produce x0 is actually better than the current "inf", and I do agree that it makes the code somewhat more readable.
I'll accept your suggestion - just making sure you understand the implications.

btw: the other fix (#998) has the advantage that the edge cases are naturally resolved, and you do not have to think about all these cases (I guess it would also produce x0 rather than inf, but I did not check that).

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

@pelson: I also prefer this over #998 so I am closing that PR.

I can see us having to make major changes to the delaunay code in the long term to improve its robustness. A complete rewrite may be the best solution. Hence I am not particulary bothered about the other testing issues highlighted in #998. @AmitAronovitch: if you can address the various line comments in this PR then I would happily commit this PR without any further widening of scope.

@AmitAronovitch

@ianthomas23: For the future code: I think we should probably use some external library (possibly qhull as scipy does) rather than maintain this here - I'll post to the mailing list later on.

I'll patch the c code in a minute (but please read my long comment above). Other stuff has already been patched, except the comment about class-methods vs than module-functions (line 189), which did not seem right to me (please reply there if you think otherwise).
Let me know if there's anything else.

@pelson, @WeatherGod ,@ianthomas23 :Thanks for the review

@AmitAronovitch

This diff made the previous one disappear (as "outdated diff"), along with the discussion leading to this patch.

I do wish reviewers to read the long comment at the end of that discussion, in which I explain the implications (the patch has some effect which might be positive, but I do not think it was intended by the people who proposed it).

The item appears as "@WeatherGod discussed an outdated diff", and you should be able to expand it and see discussion below.

@dmcdougall
Collaborator

@ianthomas23: For the future code: I think we should probably use some external library (possibly qhull as scipy does) rather than maintain this here - I'll post to the mailing list later on.

I agree with this. I recommend Jonathan Shewchuk's triangle library.

@AmitAronovitch

How can I make the auto-build try again?
This PR shows as build-failed. The message shows failure on py2.6, but from the error log message it seems like a temporary connectivity error.
On my system it merges, compiles, and passes tests successfully on Python 2.6 (as well as on 2.7). I had ignored it for some time - but it seems that the "Travis build" does not give it a second chance...

Should I make a dummy commit on this branch, or is there another way to make it try again?

@dmcdougall
Collaborator

@AmitAronovitch Not sure. Are you running the tests with Python 3.x?

@AmitAronovitch

@dmcdougall no I did not. Travis had reported success on all versions (including 3.x) except 2.6. My default Python is 2.7.
If I do not find a way to make it re-run the test, I'll add a dummy commit (add/remove some whitespace) and see if that works.

@dmcdougall
Collaborator

@AmitAronovitch No need to add a dummy commit, @mdboom worked some travis magic (7dd57d2). If you rebase this branch against current master that should trigger a travis re-build.

@AmitAronovitch

@dmcdougall - seems like that patch would only affects python 3.x (so probably unrelated to current problem). Rebasing would certainly trigger a re-build, but would probably require a whole new PR (which would make the conversation above less accessible for reviewers). Still, I don't mind doing that if it makes it easier to merge this code.
@ianthomas23 : do you think a rebase would suffice, or should I fix anything else instead/in addition?

@AmitAronovitch

@WeatherGod, @ianthomas23 : I repeat my previous comment (which was hidden in "Outdated Diff" above ) - since I see now that it is hard to follow the way it was phrased above (it was intermixed with other comments):

You had suggested that in my patch I should check for xsteps==1 rather than x1==x0.
In the case of interest (1d grid: e.g. [2:2:1j,1:10:3j] ) the two conditions are equivalent, so your proposal solves #997 identically to my original fix.
However, for the case in which x1!=x0 (e.g. [2:3:1j,1:10:3j] ), the proposed check would still generate a valid 1d grid (at x0), as opposed to the behavior of the current stable code (and my original fix), which fill the generated 1d grid with NaN values.
I think that this new behavior is actually an improvement (better to generate some deterministic solution even if it is not unique), so I had accepted your suggestion in 964a854 . However, this is still a semantic change beyond the scope of #997, so I wanted to point your attention to it.

@dmcdougall
Collaborator

@AmitAronovitch Oh, I'm sorry. There's been recent problem with numpy on Python 3.x, and I assumed this was one of them. That's my fault for not checking properly. Yes, the python26 build failure was a dud, but it passed with python27. It's probably fine, but if you want I can check the tests on my local machine with 2.6 if it'd make you feel better.

@WeatherGod
Collaborator

I think I am ok with such a change.

@ianthomas23
Collaborator

I'm OK with this PR, and it has been hanging around for too long for such a small change so I will merge it. I'm taking @dmcdougall's word that the Travis failure isn't really a problem.

@ianthomas23 ianthomas23 merged commit 267fb6b into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
4 lib/matplotlib/delaunay/_delaunay.cpp
@@ -289,8 +289,8 @@ static PyObject *linear_interpolate_grid(double x0, double x1, int xsteps,
if (!z) return NULL;
z_ptr = (double*)PyArray_DATA(z);
- dx = (x1 - x0) / (xsteps-1);
- dy = (y1 - y0) / (ysteps-1);
+ dx = ( xsteps==1 ? 0 : (x1 - x0) / (xsteps-1) );
+ dy = ( ysteps==1 ? 0 : (y1 - y0) / (ysteps-1) );
rowtri = 0;
for (iy=0; iy<ysteps; iy++) {
View
BIN  lib/matplotlib/tests/baseline_images/test_delaunay/delaunay-1d-interp.png
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
View
27 lib/matplotlib/tests/test_delaunay.py
@@ -159,7 +159,7 @@ def interpolator(self, func):
z = func(self.x, self.y)
return self.tri.nn_extrapolator(z, bbox=self.xrange+self.yrange)
-def make_all_testfuncs(allfuncs=allfuncs):
+def make_all_2d_testfuncs(allfuncs=allfuncs):
def make_test(func):
filenames = [
'%s-%s' % (func.func_name, x) for x in
@@ -186,4 +186,27 @@ def reference_test():
for func in allfuncs:
globals()['test_%s' % func.func_name] = make_test(func)
-make_all_testfuncs()
+make_all_2d_testfuncs()
+
+# 1d and 0d grid tests
+
+ref_interpolator = Triangulation([0,10,10,0],
@pelson Collaborator
pelson added a note

Rather than using the module level namespace, it might be worth considering constructing a class which has "test_" methods.

When adding tests to an existing module, I should try staying close to existing style.
make_all_testfuncs() was there (I just renamed it), and since it used module functions rather than class methods, so did I
( and that would have been my default choice anyways in this case).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ [0,0,10,10]).linear_interpolator([1,10,5,2.0])
+
+def test_1d_grid():
+ res = ref_interpolator[3:6:2j,1:1:1j]
+ assert np.allclose(res, [[1.6],[1.9]], rtol=0)
+
+def test_0d_grid():
+ res = ref_interpolator[3:3:1j,1:1:1j]
+ assert np.allclose(res, [[1.6]], rtol=0)
+
+@image_comparison(baseline_images=['delaunay-1d-interp'], extensions=['png'])
+def test_1d_plots():
+ x_range = slice(0.25,9.75,20j)
+ x = np.mgrid[x_range]
+ ax = plt.gca()
+ for y in xrange(2,10,2):
+ plt.plot(x, ref_interpolator[x_range,y:y:1j])
+ ax.set_xticks([])
+ ax.set_yticks([])
Something went wrong with that request. Please try again.