Skip to content
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

Raise error instead of attempting absurd BarbPlot #2785

Open
sgdecker opened this issue Nov 9, 2022 · 2 comments · May be fixed by #2788
Open

Raise error instead of attempting absurd BarbPlot #2785

sgdecker opened this issue Nov 9, 2022 · 2 comments · May be fixed by #2788
Labels
Area: Plots Pertains to producing plots Type: Enhancement Enhancement to existing functionality

Comments

@sgdecker
Copy link
Contributor

sgdecker commented Nov 9, 2022

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 sgdecker added the Type: Feature New functionality label Nov 9, 2022
@dopplershift dopplershift added the Area: Plots Pertains to producing plots label Nov 10, 2022
@dopplershift
Copy link
Member

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 dopplershift added Type: Enhancement Enhancement to existing functionality and removed Type: Feature New functionality labels Nov 10, 2022
@sgdecker
Copy link
Contributor Author

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 added a commit to sgdecker/MetPy that referenced this issue Nov 10, 2022
Fixes Unidata#2785.

Currently, BarbPlot is happy to attempt to plot very large wind barbs,
but if the size becomes too large, memory usage becomes an issue,
leading to system hangs.  This commit adds a check to make sure the
wind barbs will not consist of so many pennants that memory could be
an issue.
@sgdecker sgdecker linked a pull request Nov 10, 2022 that will close this issue
3 tasks
sgdecker added a commit to sgdecker/MetPy that referenced this issue Nov 17, 2022
Fixes Unidata#2785.

Currently, BarbPlot is happy to attempt to plot very large wind barbs,
but if the size becomes too large, memory usage becomes an issue,
leading to system hangs.  This commit adds a check to make sure the
wind barbs will not consist of so many pennants that memory could be
an issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Plots Pertains to producing plots Type: Enhancement Enhancement to existing functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants