Update Axes3D.tricontour for custom triangulations #1299
Conversation
| if zdir != 'z': | ||
| tri = Triangulation(jX, jY, tri.triangles, tri.mask) | ||
|
|
||
| cset = Axes.tricontour(self, tri, jZ, *args, **kwargs) | ||
| self.add_contour_set(cset, extend3d, stride, zdir, offset) | ||
|
|
||
| self.auto_scale_xyz(X, Y, Z, had_data) |
There was a problem hiding this comment.
Also, I think this is mistake. Should this be self.auto_scale_xyz(jX, jY, jZ, had_data)? Or am I overlooking something?
There was a problem hiding this comment.
IIRC, it is correct. What the rotate_axes does is switches up which X/Y/Z data is assigned to which jX,jY,jZ variables, but only for the purpose of the contouring process. We want the limits to be determined from the original data. Note, I have never really thought about this question, and I could be completely wrong.
There was a problem hiding this comment.
So, the result of rotate_axes is that the data jX gets plotted on the x-axis. The data jY on the y-axis and the data jZ on the z-axis. I think the x-axis data limits should be determined by the data that is plotted on them, i.e. jX, not X.
I think the auto_scaling on X, Y and Z will look wrong for data stretching longer in one direction than another.
There was a problem hiding this comment.
I just tried it out with contourf3d_demo2.py. The autoscaled limits should be: x [-30, 30], y [-30, 30], z [-80, 80]. If you take out the calls to set_*lim(), take out the call to plot_surface(), and leave only the call to the second contourf (and use offset=-30), you will see that the autoscaling works fine (if you use my branch at PR #1289). If you change the call in contourf3d() to use jX, jY, and jZ for autoscaling, it comes out stretched in the z-direction.
jX, jY, and jZ are juggled around so that the contouring data that was provided in, for example, Y, ends up in jZ, but this is only for the sake of contouring. The contouring code expects the first two arguments to be coordinate data, while the third is the height data to contour. If dir='y', then the data in Y is the height data, and Z is the coordinates. jY becomes the coordinates of Z, and jZ gets the values in Y, but then everything gets flipped back around when 'cset' is passed into add_contour_set().
The current way of calling auto_scale_xyz() is correct, it is just not functioning properly due to other issues that are fixed with PR #1289.
|
@WeatherGod - I'm not sure on the status of this PR. Would you mind having another look over to determine whether this PR is go/no-go? Thanks, |
|
@pelson This PR needs feedback on what the best course of action to take is regarding the comment in the code referring to a possibly wasted computation of a triangulation. It's been a while since I have looked at this code, though. |
|
@dmcdougall: Apologies for not picking this up earlier. I am a bit confused by the code and comments as they currently stand. You certainly should not recompute a triangulation, but if the line you are concerned about is However, I've only had a cursory glance at the code and I don't really understand the axes juggling, so I may have completely misunderstood what is intended. |
|
@ianthomas23 Ahh, of course. I should have realised that. In which case, this is good to go I think. Docs and a code-tidy are imminent. |
|
Actually, while I'm at it I might as well do |
|
Done. |
|
Looks great. Maybe add an image comparison test? |
| @@ -1,3 +1,6 @@ | |||
| 2013-03-31 Added support for arbitrary unstructured user-specified | |||
| triangulations to Axes3D.contour[f] - Damon McDougall | |||
|
@dmcdougall: I probably shouldn't have encouraged you to work on this! The output from I don't really know what the best approach is here. This PR won't render tricontourf correctly in 3D, but then the existing code doesn't either and at least this PR is more functionally complete than the existing code. |
|
I still haven't figured out #178. I have looked up and down the code path, On Thu, Apr 4, 2013 at 12:25 PM, Ian Thomas notifications@github.comwrote:
|
|
Are the stoppers on this PR, or is it good to go? I think we should try to either merge this or close this PR before |
|
Also, if you wouldn't mind rebasing so we can get Travis to run over this again. |
|
@pelson: The situation with this PR is that the code changes are good, but the output is rendered incorrectly due to an old bug, or strictly speaking missing functionality, in mplot3d (#178). I have diagnosed the cause of the bug and tried to produce a PR that is functionally correct but stomps all over the mplot3d code (#1928) so it needs more work. To progress, that bug fix needs to be completed by someone who understands the mplot3d code base and usage (or they could start again from scratch if they wish of course). You could argue that this PR should be merged as it is more functionally complete than the existing code, and although it renders incorrectly so potentially do any uses of Poly3DCollection in mplot3d. However, if an image comparison test is added as recommended by @mdboom, the image would have to be of the incorrectly-rendered output for the test to pass! |
|
I am in favor of merging this in as-is, so long as there is a big "Experimental" tag in the docstrings. |
|
Ok, here's my two cents. A test is always a good idea. That said, I feel a little dirty writing a test that will fail on correct output (a la @ianthomas23's comment). I'll rebase and add some disclaimers in the docstrings, as per @WeatherGod's suggestions. I'll leave the decision to merge by someone other than myself. |
An example has also been added to illustrate the additional flexibility possible with providing a user-defined triangulation.
|
Done. |
|
@ianthomas23 - are you willing to back this PR given your substantial changes to the triangulation functionality recently? If not, please close the PR for completeness. Cheers, |
|
Given #178 is from 2010 I'm not inclined to keep this PR hanging around until it is solved. Looking at this myself, I'm only seeing that this is extending the interface, not actually exacerbating the problem. And a strong point in its favour is that it does actually add the fact that the whole interface (Axes3d.tricontrouf) is experimental. One that front, I'm comfortable merging this in principle. @ianthomas23 - I'll give you a chance at a veto, but I'm prepared to merge this on Monday if there is no objection. Cheers! |
Update Axes3D.tricontour for custom triangulations
An example has also been added to illustrate the additional flexibility possible with providing a user-defined triangulation.
There has been a snag. There is no neat way, if the user specifies a non-default
zdir, to triangulate in the correct coordinate plane. This solution basically re-triangulates the points in the correct plane if azdirother thanzis provided.I don't like this solution. The alternative would be to add a convenience method to
matplotlib.Triangulateto return thex, y, zdata coordinates without computing a, possibly wasted, triangulation. This, however, is 3d specific. No axes-juggling happens in 2d so they don't need to be rotated.I thought I would stop here with this solution before continuing onto
Axes3D.tricontourfNote: This pull request is not complete, it requires feedback before it can be finished.