skip_patches_outside_xylimits doesn't work with grid mappings
rjleveque opened this issue · 9 comments
The new feature added in #262 skips plotting AMR patches that are outside the specified xlimits and ylimits, unless plotaxes.skip_patches_outside_xylimits == False as provided in #263. By default this is set to True.
I recently discovered that this doesn't work well with a mapped grid where a mapc2p function is provided to map the computational xc, yc to the physical space xp, yp for plotting. The problem is that xlimits, ylimits are specified in physical coordinates (what you see in the plot) whereas the check on whether to skip a patch is done before mapc2p is applied, and hence the xc ,yc used in the test are in computational coordinates. A patch may easily appear to be outside the limits even though after the mapping it will be inside. This is also a problem in 1d if mapc2p is specified to map xc to xp.
This would be fairly easy to fix if mapc2p were also a ClawPlotAxes attribute, and it seems like it probably should be since the axes should presumably be in the same coordinate system regardless of what item is being plotted on it.
However, for some reason (my bad design, probably), mapc2p is an attribute of a ClawPlotItem, or if that is not set, the code also checks for mapc2p as an attribute of plotdata, but there is no plotaxes.mapc2p.
Cleaning this up will take some work and needs further discussion.
In the meantime, users should be aware that if using a mapc2p function, you should also set plotaxes.skip_patches_outside_xylimits to False.
Should this be the default, rather than True?
I would favor keeping the default as True myself.
The reasoning is that there are very few who use mapped grids to plot and I would consider those who do to be the experts. I also think that the speedup that this enables for the generic GeoClaw user, who is more than likely not an expert, would be well worth the issue as @rjleveque outlined it. Furthermore I think that adding mapc2p to the plotting capabilities is something we should enforce as was done in the old Matlab plotting code.
Of course just my 2¢.
@mjberger oh man, if you are not an expert are any of us except for maybe @rjleveque 😉
I won't say how long it took me to figure out why nothing showed up in my plots with a mapped grid the other day, but much too long. So I agree with @mjberger that True is not a good default in general. On the other hand for many GeoClaw applications with zoomed plots are small regions from big computations, it is nice to have it skip patches by default. I've got a quick and dirty fix that I think sets the default to True unless there's a mapc2p function for any item in which case it's set to False. We can consider to discuss better long-term options.
I like the idea you had in the PR myself.
Do you think we should raise a warning maybe?
Where would we raise it, whenever skip_patches_outside_xylimits is set to True by default? That might get tiresome?
Note there's a possible warning message in the file that's not currently activated but could be, and would be even more tiresome since it would print a message for every axes in every frame if patches are being skipped...
if False and num_skipped > 0:
# possible warning message:
print('Skipped plotting %i patches not visible' % num_skipped)
Not sure, I was thinking maybe if someone set plotaxes.skip_patches_outside_xylimits = True, had a mapped grid and did not specify a mapc2p that we should raise a warning.