MetPy icon indicating copy to clipboard operation
MetPy copied to clipboard

Raise error instead of attempting absurd BarbPlot

Open sgdecker opened this issue 2 years ago • 2 comments

What should we add?

The following code is a great way to eat through all of your system's memory and then some as Matplotlib attempts to draw trillions of triangles:

import xarray as xr
from metpy.cbook import get_test_data
from metpy.plots import BarbPlot, MapPanel, PanelContainer

data = xr.open_dataset(get_test_data('narr_example.nc')).squeeze()

bp = BarbPlot()
bp.data = data
bp.field = ['u_wind', 'v_wind']
bp.level = 500
bp.scale = 1e14

mp = MapPanel()
mp.layout = (1, 1, 1)
mp.layers = ['coastline', 'borders', 'states']
mp.plots = [bp]

pc = PanelContainer()
pc.size = (10, 8)
pc.panels = [mp]
pc.show()

(This report is inspired by true events.)

My feature request is that the declarative interface raise an error when the magnitudes are so high that the number of pennants drawn would exceed some reasonable threshold, rather than attempt to create the plot and hang the computer.

Reference

No response

sgdecker avatar Nov 09 '22 22:11 sgdecker

My initial thoughts:

  • What's the surefire threshold that in all cases we should cut off? Because since there's no escape hatch if we stop, we better be 100% sure we're not blocking someone's intentional use case.
  • Should this be in upstream matplotlib instead?

My gut reaction wonders if doing this would run afoul of the Zen of Python's "In the face of ambiguity, refuse the temptation to guess." and Python's "consenting adults" nature.

dopplershift avatar Nov 10 '22 18:11 dopplershift

This variation will run without coming anywhere close to consuming all memory, and yet I can't think of any way the resulting map would be useful or intentional in a meteorological context:

import xarray as xr
from metpy.cbook import get_test_data
from metpy.plots import BarbPlot, MapPanel, PanelContainer

data = xr.open_dataset(get_test_data('narr_example.nc')).squeeze()

bp = BarbPlot()
bp.data = data
bp.field = ['u_wind', 'v_wind']
bp.level = 500
bp.scale = 1e3
bp.skip = [8, 8]

mp = MapPanel()
mp.layout = (1, 1, 1)
mp.area = [-100, -70, 30, 45]
mp.layers = ['coastline', 'borders', 'states']
mp.plots = [bp]

pc = PanelContainer()
pc.size = (10, 8)
pc.panels = [mp]
pc.show()

wind

I would propose something like, if at least half of the wind barbs about to be drawn, after performing any unit conversions, exceed a magnitude of 50,000 (= 1000+ pennants on at least half the barbs), abort. I suppose this could go upstream instead, but there is the caveat that matplotlib has a more general purpose than the MetPy declarative interface, so perhaps there is some reason someone would want to abuse plt.barbs outside the context of meteorology. In addition, plt.barbs has a barb_increments optional argument that could be used to "cancel out" the issue (by, for instance, setting 'flag' to 1e14, etc.), and that would need to be accounted for in any change to matplotlib itself.

Regarding consenting adults, the context for that phrase seems to have more to do with accessing "private" class attributes. In any case, since a beginner student was involved in the case at hand here, I would say not all MetPy users are at the age of consent metaphorically speaking.

sgdecker avatar Nov 10 '22 21:11 sgdecker