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

Vector support in bridge.Transform.from_points #676

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions src/geovista/bridge.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
ZLEVEL_SCALE,
nan_mask,
to_cartesian,
vectors_to_cartesian,
wrap,
)
from .crs import WGS84, CRSLike, to_wkt
Expand Down Expand Up @@ -545,6 +546,9 @@ def from_points(
zlevel: int | ArrayLike | None = None,
zscale: float | None = None,
clean: bool | None = None,
vectors: ArrayLike | tuple(ArrayLike) | None = None,
vectors_array_name: str | None = None,
vectors_z_scaling: float | None = None,
) -> pv.PolyData:
"""Build a point-cloud mesh from x-values, y-values and z-levels.

Expand Down Expand Up @@ -582,6 +586,18 @@ def from_points(
clean : bool, optional
Specify whether to merge duplicate points. Defaults to
:data:`BRIDGE_CLEAN`.
vectors : tuple(ArrayLike), optional
If present, a tuple of 2 or 3 arrays of the same shape as `xs` and `ys`.
These give eastward, northward and (optionally) vertical vectors, which are
converted to an [N, 3] array of 3-D vectors attached to the result as a
points array ``mesh["vectors"]``. This can be used to generate glyphs
(such as arrows) and streamlines.
vectors_array_name : str, optional
Specifies an alternate name for the points array to store the vectors.
Also set as the active vectors name. Defaults to "vectors".
vectors_z_scaling : float, optional
scaling factor to apply to vertical vectors (i.e. relative to the eastward
and northward components). Defaults to 1.0

Returns
-------
Expand Down Expand Up @@ -636,6 +652,42 @@ def from_points(
mesh.field_data[GV_FIELD_NAME] = np.array([name])
mesh[name] = data

if vectors is not None:
if vectors_array_name is None:
vectors_array_name = "vectors"

if not isinstance(vectors, tuple) or not all(
isinstance(arr, np.ndarray) for arr in vectors
):
msg = 'Keyword "vectors" must be a tuple of array-like.'
raise ValueError(msg)

n_vecs = len(vectors)
if n_vecs not in (2, 3):
msg = (
'Keyword "vectors" must be a tuple of 2 or 3 arrays : '
f"got {n_vecs}."
)
raise ValueError(msg)

vectors = [np.asanyarray(vecdata) for vecdata in vectors]
xx, yy = vectors[:2]
zz = vectors[2] if n_vecs > 2 else np.zeros_like(xx)

if vectors_z_scaling is not None:
zz = zz * vectors_z_scaling
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that there is a case for vectors_to_cartesian to be used in other transforms like from_unstructured, would it be worth considering if this code (from line 659 to here) would be suitable to exist inside vectors_to_cartesian? It doesn't seem to me like this code is specific to from_1d and I don't believe there's anything here which wouldn't work with the other transforms from what I've seen of them.

Copy link
Collaborator Author

@pp-mo pp-mo Jan 30, 2024

Choose a reason for hiding this comment

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

Interesting...
I'm actually doubting right now whether we may not remove this keyword altogether, since it's so obvious how the user would do it.
I already removed an overall scaling operation for the same reason, and also because it can be done in the 'glyph' call.


# TODO @pp-mo: should we pass flattened arrays here, and reshape
# as-per the inputs
# (and xyz)? not clear if multidimensional input is used or needed
xx, yy, zz = vectors_to_cartesian(
lons_lats=(xs, ys),
vectors_uvw=(xx, yy, zz),
)
vectors = np.vstack((xx, yy, zz)).T
mesh[vectors_array_name] = vectors
mesh.set_active_vectors(vectors_array_name)

# clean the mesh
if clean:
mesh.clean(inplace=True)
Expand Down
45 changes: 45 additions & 0 deletions src/geovista/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -750,6 +750,51 @@ def to_cartesian(
return np.vstack(xyz).T if stacked else np.array(xyz)


def vectors_to_cartesian(
lons_lats: (ArrayLike, ArrayLike),
vectors_uvw: (ArrayLike, ArrayLike, ArrayLike),
) -> (np.ndarray, np.ndarray, np.ndarray):
"""Convert geographic-oriented vectors to cartesian ``xyz`` points.

Parameters
----------
lons_lats : pair of ArrayLike
The longitude + latitude locations of the vectors (in degrees).
Both shapes must be the same.
vectors_uvw : triple of ArrayLike
The eastward, northward and upward vector components.
All shapes must be the same as in ``lons_lats``.

Returns
-------
(ndarray, ndarray, ndarray)
The corresponding ``xyz`` cartesian vector components.

Notes
-----
.. versionadded:: 0.?.?
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume this gets gets fixed once the PR is completed/approved.


"""
# TODO @pp-mo: Argument checking ???
lons, lats = (np.deg2rad(arr) for arr in lons_lats)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It may be outside the scope of this PR, but it occurs to me that the documentation of these functions could be a bit clearer on the fact that, unless a crs is specified, units are expected to be in degrees. Though I suppose this may be implicit in the default crs being WGS 84.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I felt that all the existing docs of Transform methods are missing any statement of this, so it just didn't seem an appropriate place to mention it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, not true I see : in the dosctring here for from_points, it does say that xs and ys are in crs units.
However, this routine absolutely will not work with non-latlon coordinates.
So perhaps I can improve on the checking for that ...

u, v, w = vectors_uvw

coslons = np.cos(lons)
sinlons = np.sin(lons)
coslats = np.cos(lats)
sinlats = np.sin(lats)
# N.B. the term signs are slightly unexpected here, because the viewing coord system
# is not quite what you may expect : The "Y" axis goes to the right, and the "X"
# axis points out of the screen, towards the viewer.
z_factor = w * coslats - v * sinlats
wy = coslons * u + sinlons * z_factor
wx = -sinlons * u + coslons * z_factor
wz = v * coslats + w * sinlats
# NOTE: for better efficiency, we *COULD* handle the w=0 special case separately.

return wx, wy, wz


def to_lonlat(
xyz: ArrayLike,
radians: bool | None = False,
Expand Down
Loading