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
Cubic interpolation for triangular grids #1786
Conversation
This looks like an excellent piece of work. Inevitably, nobody will be able to review this line for line, but it looks well tested, well exampled and from a quick glance, well documented. @ianthomas23 - I know you've been very close to the implementation, but even so I'd love to hear from you regarding this work. @GBillotey - how much of this work is new vs modifying previously existing functionality? I'd like to get a feel for the potential impact that this could have on current users. @mdboom - where do we stand on this? Clearly @GBillotey & @ianthomas23 are domain experts in this implementation - personally, I'd be happy to merge this early given how much thought and effort has clearly been put in. Good stuff guys! |
It would be great to add a few regression tests (it's probably fine if they are PNG only) perhaps based on these included examples. |
@pelson: I agree that it is an excellent piece of work. @GBillotey started off with some rough code to scratch his particular itch and has evolved it into something that is well organised and robust enough to be included in matplotlib. The examples are particularly good; one especially is more like a mini-tutorial than just an example plot. I have reviewed the code periodically and in my last review it was difficult to find any errors other than typos in the documentation. I would like to check it over one more time just in case, but after that I would be happy to accept it. I'll answer your question to @GBillotey whilst I am writing. This is all new work, following on from my linear triangular grid interpolator PR of 2 months ago (#1582). There are some changes to LinearTriInterpolator to better structure the code, fix a bug and improve performance, but no change to the functionality or API. There should be no impact to existing users. |
cubic_geom = mtri.CubicTriInterpolator(triang, z, kind='geom') | ||
zs = quad(xs, ys) | ||
for interp in (cubic_min_E, cubic_geom): | ||
diff_lin = np.abs(linear_interp(xs, ys) - zs) |
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 line can go before the previous line.
I have added 2 PNG regression tests and taken into account @ianthomas23 comments. I will wait a few days before actually pushing the changes, just in case somebody else wants to comment. |
Very good. One minor point: for the tests, can you add |
Could you make sure your code is pep8 and follows the numpydoc conventions? I'll pinpoint a couple of "unpep8ness" in the code. |
|
||
Parameters | ||
---------- | ||
triangulation : :class:`~matplotlib.tri.Triangulation` |
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.
You can write ~matplotlib.tri.Triangulation
directly: sphinx will link automatically to the class.
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.
Really? Do you happen to know the rules of this namespacing? I couldn't find it in the docs of http://sphinx-doc.org/domains.html#the-python-domain or http://sphinx-doc.org/domains.html#info-field-lists...
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 it's some magic from numpydoc but I'm not sure.
On sklearn, we even have some magic happening in the examples: it links all methods from the source code to the documentation, including matplotlib and numpy methods: http://scikit-learn.org/dev/auto_examples/plot_classification_probability.html
I don't know how we do that, but I find it very cool :)
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.
Isn't it the so-called "default role" of Sphynx > 0.4 http://sphinx-doc.org/config.html#confval-default_role ?
@NelleV : is it a "can" or a "should" ?
I understand from MEP10 that all cross-references will have at some point to be updated "in a semi-automated way" with this format (provided this default role doesn't get ambiguous, though).
Note that I would be happy to do it if it is the recommandation, I am just asking.
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.
@GBillotey it is a "can" it terms of sphinx/numpydoc, and a "should" in terms of MEP10, but I think it should be a roadblock for merging this patch.
I am happy with this PR and am ready to merge it, but will wait a couple of days in case there are any final objections. All the comments have been addressed, in particular image tests have been added and the code is sufficiently MEP10 compatible. I have run pep8 on all the source files, and the only issues raised are not enough whitespace around operators and too much whitespace around square brackets when constructing lists/arrays. The former corresponds to situations where pep8 is overzealous in interpreting the standard, and the latter has been done on purpose so that columns of numbers are vertically aligned, which is much more readable than strictly following PEP8. The Travis failure appears to be a dud. |
Though it's not strictly related to this PR, I've been meaning to ask if you have any idea how much of this work could be applied to spherical triangulation? Is that a completely different beast, or do you think there could be some significant overlap? Great work! |
@pelson Thanks! This "plane" interpolator should provide a good starting point for a spherical triangulation interpolator. My feeling is that the quantity of additional code will depend on the typical size of the data you want to interpolate.
One additional point to consider: is your final goal plotting contours or interpolating. If you decide to give a try, do not hesitate to email me. |
Cubic interpolation for triangular grids
This is the second and final PR for issue #1521.
It adds a
CubicTriInterpolator
to interpolate scalar fields defined on triangular grids, and a utility classUniformTriRefiner
to perform high-resolution tricontouring by grid refinement.A utility class
TriAnalyzer
is also added, which performs basic triangular mesh analysis.I have included tests for the 3 classes, 3 pylab_examples which describe the functionalities, and have updated sphinx docs, whats_new and CHANGELOG.
Regarding the documentation, I did my best to follow MEP10 ; any comment from MEP10 specialists is highly appreciated.
(I make here the observation that, as of Numpydoc 0.4, a
__call__
method is not exposed in the documentation unless explicitly listed. Also refer to numpy/numpy@7a57dc1).This is my first matplotlib submission; my warmest thanks goes to @ianthomas23 for his very detailed instructions throughout the process. Any remaining mistake is mine.