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

Add WindBarbs to Matplotlib Backend #651

Merged
merged 15 commits into from
Jul 6, 2023
Merged

Add WindBarbs to Matplotlib Backend #651

merged 15 commits into from
Jul 6, 2023

Conversation

ahuang11
Copy link
Collaborator

@ahuang11 ahuang11 commented Jun 9, 2023

Still working on it. Also, I'm still on the fence whether this should reside in GeoViews vs HoloViews.

Closes #133

Wondering if this should follow VectorField conventions (magnitude and angle) instead of U, V

I'm leaning towards U and V because in my experience dealing with atmospheric science data, files are always separated by zonal and meridional winds (U and V).

The downside of not following VectorField conventions is that users wont be able to swap out gv.Barbs for gv.VectorField directly.

@jbednar
Copy link
Member

jbednar commented Jun 9, 2023

whether this should reside in GeoViews vs HoloViews.

It would be odd to have something as specific as "WindBarbs" be in HoloViews; for it to fit semantically into HoloViews I think it would have to have a name and an interpretation that is not specific to wind. E.g. it could be something about representing additional dimensions in a VectorField, with an aside that suggests that it's common to do that in meteorology. If it's in HoloViews, following the VectorField conventions would be appropropriate.

Another reason to put it into GeoViews is that I don't believe HoloViews currently includes any custom Bokeh models or extensions, while implementing this for Bokeh will probably require those (which is ok for GeoViews).

@jbednar
Copy link
Member

jbednar commented Jun 9, 2023

I'm leaning towards U and V because in my experience dealing with atmospheric science data, files are always separated by zonal and meridional winds (U and V).

Maybe there could be an alternative constructor (or constructor arguments) to convert incoming data from whichever of the two conventions is not the default. If data is natively in u,v it does make sense to keep it that way to avoid having to convert and store the data for most use cases.

@ahuang11
Copy link
Collaborator Author

ahuang11 commented Jun 9, 2023

Good idea; I think we can do

gv.WindBarbs.from_wind_components(X, Y, U, V)

@ahuang11
Copy link
Collaborator Author

Okay, I just added a from_uv_components param

@ahuang11 ahuang11 marked this pull request as ready for review June 12, 2023 02:39
@ahuang11 ahuang11 changed the title WIP: Add WindBarbs to Matplotlib Backend Add WindBarbs to Matplotlib Backend Jun 12, 2023
@ahuang11 ahuang11 requested a review from hoxbro June 15, 2023 17:30
Copy link
Member

@hoxbro hoxbro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have added some comments.

Some of them are trigonometry related. So stupid questions can occur. 🙂

I think the failing Mac test can be fixed by merge the main.

geoviews/plotting/mpl/chart.py Outdated Show resolved Hide resolved
geoviews/plotting/mpl/chart.py Outdated Show resolved Hide resolved
geoviews/plotting/mpl/chart.py Outdated Show resolved Hide resolved
geoviews/plotting/mpl/chart.py Outdated Show resolved Hide resolved
@ahuang11
Copy link
Collaborator Author

ahuang11 commented Jun 27, 2023

I refactored based on https://github.com/Unidata/MetPy/blob/main/src/metpy/calc/basic.py#L62

To verify my implementation, I compared using metpy + matplotlib:

# metpy barbs

import numpy as np
import metpy.calc as mpcalc
from metpy.units import units
import matplotlib.pyplot as plt

x = np.linspace(-1, 1, 4)
y = np.array([0, 1])
X, Y = np.meshgrid(x, y)
U, V = 10 * X, 0 * Y

mag = mpcalc.wind_speed(U * units("m/s"), V * units("m/s"))
angle = mpcalc.wind_direction(U * units("m/s"), V * units("m/s"))
us, vs = mpcalc.wind_components(mag, angle)

fig = plt.figure(figsize=(7, 15))
ax1 = plt.subplot(311)
ax1.barbs(X, Y, U, V)

ax2 = plt.subplot(312)
ax2.barbs(X, Y, us, vs)
ax2.set_title("UV metpy")

ax2 = plt.subplot(313)
ax2.barbs(Y, X, vs, us)
ax2.set_title("invert axes")

My implementation

import numpy as np
import pandas as pd
import geoviews as gv

gv.extension("matplotlib")

x = np.linspace(-1, 1, 4)
y = np.array([0, 1])
X, Y = np.meshgrid(x, y)
U, V = 10 * X, 0 * Y

angle = (np.pi / 2) - np.arctan2(-V, -U)
mag = np.hypot(U, V)

barbs = gv.WindBarbs((X, Y, angle, mag))
from_uv_barbs = gv.WindBarbs.from_uv((X, Y, U, V))
(barbs + from_uv_barbs + from_uv_barbs.clone().opts(invert_axes=True)).cols(1).opts(fig_inches=10)

Matplotlib
image

GeoViews
image

@ahuang11 ahuang11 requested a review from hoxbro June 27, 2023 21:45
geoviews/element/geo.py Outdated Show resolved Hide resolved
geoviews/tests/plotting/mpl/test_chart.py Outdated Show resolved Hide resolved
geoviews/tests/plotting/mpl/test_chart.py Outdated Show resolved Hide resolved
geoviews/tests/plotting/mpl/test_chart.py Outdated Show resolved Hide resolved
Copy link
Member

@hoxbro hoxbro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just in case you did not see this comment, I think we should update: https://geoviews.org/gallery/bokeh/orthographic_vectorfield.html#declare-data with the same calculations as used in here.

geoviews/element/geo.py Outdated Show resolved Hide resolved
geoviews/tests/plotting/mpl/test_chart.py Outdated Show resolved Hide resolved
@ahuang11
Copy link
Collaborator Author

ahuang11 commented Jul 6, 2023

Thanks for correcting me about the data mutation; that makes sense.

Here, we aren't actually doing any additional calculations on the original data; we simply reorganize the first two items to use angles and magnitudes. So instead of naming it transformed data, I renamed it to reorganized data and left a comment.

@ahuang11 ahuang11 merged commit f114de0 into main Jul 6, 2023
@ahuang11 ahuang11 deleted the add_windbarbs branch July 6, 2023 13:04
@ahuang11
Copy link
Collaborator Author

ahuang11 commented Jul 6, 2023

Woooo

@philippjfr
Copy link
Member

Sorry for not reviewing this earlier, and I might be missing something but does this implement proper projection support for the barbs?

@ahuang11
Copy link
Collaborator Author

ahuang11 commented Jul 6, 2023

Probably not; I was just looking at #296

I can test it out soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a Barbs element
4 participants