-
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Linear tri interpolator #1582
Linear tri interpolator #1582
Conversation
In the absence of comments from anyone else, I will check this in myself. I'll leave it a few days more in case there are any objections. |
Just gave this a visual inspection, and the code looks very well commented and fairly easy to follow. Good job, @ianthomas23 |
} | ||
} | ||
|
||
return Py::asObject((PyObject*)planes_array); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function should probably be wrapped in a try
... catch
to ensure that Py_DECREF(z) is called. It seems unlikely an exception would get thrown here, but a future change may introduce something that could.
Looks good on quick experimentation. 👍 from me. |
# Create triangulation. | ||
x = np.asarray([0, 1, 2, 3, 0.5, 1.5, 2.5, 1, 2, 1.5]) | ||
y = np.asarray([0, 0, 0, 0, 1, 1, 1, 2, 2, 3]) | ||
triangles = [[0,1,4], [1,2,5], [2,3,6], [1,5,4], [2,6,5], [4,5,7], [5,6,8], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiny nitpicks: this file is not completely pep8 compliant: there should be spaces after ,
, and spaces around *
(l. 18)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this line is more readable without the spaces after commas, but as I am not particularly bothered either way I am happy to change it.
No extras spaces are needed around any of the *
in the line
z = np.cos(1.5*x)*np.cos(1.5*y)
From PEP8: "If operators with different priorities are used, consider adding whitespace around the operators with the lowest priority(ies). Use your own judgement". There are no operator priorities causing confusion here; the line is fine as it is. I assume you are using the pep8
tool which is sometimes a little too enthusiastic - it's recommendation of
z = np.cos(1.5 * x) * np.cos(1.5 * y)
is very poor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have to use the pep8 tool to see this isn't pep8. I prefer z = np.cos(1.5 * x) * np.cos(1.5 * y)
above z = np.cos(1.5*x)*np.cos(1.5*y)
This is a matter of choosing whether matplotlib follows strict pep8 convention or not. I always follow the strict pep8 conventation, and many scientific python projects do (with the pep8 tool or flake8 to check the compliance). I believe matplotlib should do the same (it avoids the long discussions on whether X is better than Y and the numerous commits to reformat the code such as the ones I've been doing recently on matplotlib's codebase), but I don't think there has been a clear decision on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are missing the point. I quoted the line from PEP8 that specifically states "use your own judgement". It is impossible to strictly follow the PEP8 document without exercising judgement, i.e. there is no definite right or wrong answer here.
The coding guidelines state that I should follow PEP8. I have. If you think the coding guidelines should be changed to follow pep8 (the tool, not the document) then you should start such a discussion on the matplotlib-devel mailing list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On 2013/01/08 6:47 AM, Varoquaux wrote:
In examples/pylab_examples/triinterp_demo.py:
@@ -0,0 +1,36 @@
+"""
+Interpolation from triangular grid to quad grid.
+"""
+import matplotlib.pyplot as plt
+import matplotlib.tri as mtri
+import numpy as np
+import math
+
+
+# Create triangulation.
+x = np.asarray([0, 1, 2, 3, 0.5, 1.5, 2.5, 1, 2, 1.5])
+y = np.asarray([0, 0, 0, 0, 1, 1, 1, 2, 2, 3])
+triangles = [[0,1,4], [1,2,5], [2,3,6], [1,5,4], [2,6,5], [4,5,7], [5,6,8],I don't have to use the pep8 tool to see this isn't pep8. I prefer |z =
np.cos(1.5 * x) * np.cos(1.5 * y)| above |z = np.cos(1.5_x)_np.cos(1.5*y)|
Nelle, please look at the examples below the lines in PEP 8 that Ian
quoted. They make it perfectly clear that PEP 8 does not mandate spaces
around binary operators. Personally, in this case I think the most
readable option would be to put spaces only around the middle operator.
The goal is clarity and readability, which sometimes requires
judgement. There is no such thing as "strict PEP 8 compliance"; it is
an oxymoron.
Eric
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very recent change in PEP8 I was not aware of.
Else, yes, there are stricter ways to consider the document than others (I don't consider Twisted as following PEP8, and others might). I also believe the less the developper need to think and the more tools are here to do the work and check whether the code is valid, the better it is. When it comes to styling, it's not a matter of judgement, but of culture and personal preferences, which one should generally avoid in a project.
A fix has been integrated to pep8 (the tool) to reflect the change in the PEP.
This is the first PR for issue #1521. It adds a
TrapezoidMapTriFinder
that is used to determine the triangles in which (x,y) points lie and aLinearTriInterpolator
to interpolate scalar fields defined on triangular grids. Have included tests for both classes, a pylab_example for the interpolator and an whizzy interactive event_handling example for the trifinder, and have updated sphinx docs, whats_new and CHANGELOG.I'll repeat the observation from #1521 that the delaunay module already has a linear interpolator, but it cannot be used with user-specified triangulations unlike the one in this PR which can be used with user-specified and delaunay-calculated triangulations. The new one is much more flexible in accepting arrays of points to iterate over as they can be any shape, e.g. 1D for a transect through a triangulation, 2D to create a quad mesh to pass to contourf, etc.
It is possible that the TrapezoidMapTriFinder is overkill for what we need. It is O(log N) but may be outperformed by a simpler 'walking across the triangulation' algorithm if most users are passing in hi-res regular 2D grids to interpolate to. My preference is to release this code out into the wild as it is and get feedback from users before adding/modifying the trifinder.