Vector support in bridge.Transform.from_points #676

pp-mo · 2024-01-26T10:55:35Z

POC for an API extension to gv.Transform.from_points to accept wind-like vectors and convert to xyz vectors stored on the mesh, so they can be usedwith ".glyph()" and ".arrows" usage.
(cf. PyVista glyphs examples )

~~I have a couple of draft exercises for this working,~~
~~but I don't (yet) have permission to share the data.~~
Now includes a gallery example, loading data from geovista-data (already added).

TODO:

provide suitable test data via geovista-data and pantry routine
provide example showing arrows plotting.
because it's not too trivial, this probably wants demonstrate how to do some of these things ...
- scale arrows or not (i.e. direction-only)
- ~~colour arrows by scale (or not)~~ (not needed ?)
- ~~filter out missing points~~ (not needed?)
work out whether/how data with extra dimensions might be supported (cf. Allow an additional dimension for data in geovista.Transform.from_1d etc. #554)
- spoiler : in this case, I don't see how "from_points" really extends to multi-dimensional 'xs' and 'ys',
  - so maybe it's not realistic for this routine, or we can accept any shape but will simply flatten
  - the existing docstring seem to leave it open as a possibility, but that could be just a copy+paste carryover

N.B. I've found that, to get the control you want over appearance, you will want to combine the feature with controls in ".glyph()" and ".add_mesh()" calls.

…points.

welcome · 2024-01-26T10:55:37Z

🚀 Awesome! Your first pull request! Thanks for contributing to geovista! 🚀

tkoyama010 · 2024-01-27T03:41:55Z

pre-commit.ci autofix

for more information, see https://pre-commit.ci

pp-mo · 2024-01-29T10:29:35Z

Mini status update :

it's really noticable that I have now removed several of the extra controls I originally wrote into this (like a "vectors_scaling" keyword), since I found that the facilities needed were already available in the standard PyVista calls.

There are now basically 3 separate steps, where you get to control different aspects :

in the 'from_points' call : keywords vectors + vectors_array_name and vectors_z_scaling
in the glyph call, you control scaling, factor and tolerance
in the add_mesh call, you control color and cmap

Another common need will be to threshold the mesh before calling 'glyph', to reduce the size of the results.
For that, we may want to add our own "vectors Magnitude" array -- I think this is generated by the 'glyph' call, but it is also --frustratingly-- exactly what we needed to threshold on.

So now I'm aiming to write an example to demonstrate how these various controls work together.
Possibly 2 examples, if it's too complicated for one.

stephenworsley

Looking good so far, just a couple odd comments to make here.

stephenworsley · 2024-01-29T15:14:43Z

src/geovista/bridge.py

+            zz = vectors[2] if n_vecs > 2 else np.zeros_like(xx)
+
+            if vectors_z_scaling is not None:
+                zz = zz * vectors_z_scaling


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.

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.

stephenworsley · 2024-01-29T15:20:58Z

src/geovista/common.py

+
+    Notes
+    -----
+    .. versionadded:: 0.?.?


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

stephenworsley · 2024-01-29T17:01:37Z

src/geovista/common.py

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


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.

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.

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 ...

pp-mo · 2024-01-30T11:31:04Z

Update :

Added a draft code example.
The cutdown source data is stored in a temporary path -- once this is solid I'll work on moving it to geovista-data, and use that here (via pooch + the cache)

I'm hoping the docs build will succeed, and I'll check what it looks like...

src/geovista/pantry/data.py

pp-mo · 2024-01-30T16:17:51Z

Data coming here : bjlittle/geovista-data#37

When that lands, I will update + take this out of draft.

pp-mo · 2024-01-30T17:32:50Z

Ok we now have a functioning example in the docs build, and I consider that ready to review.

However, there are some outstanding comments above,
and we have some CI problems to address
-- (not clear if those were all triggered by mergeback from main, or incomplete merges from the same ?)

bjlittle · 2024-01-30T21:42:09Z

@pp-mo The CI failures are due to image test failures.

All examples will automatically undergo image testing to ensure that they are not broken.

We simple need to add an expected image for the tests. I can do that for you, and then talk you through the steps so that you know next time.

Ultimately, such things will all be captured in developer documentation ... but we need to let the dust settle first before doing that 👍

tkoyama010 · 2024-02-05T02:53:56Z

pre-commit.ci autofix

for more information, see https://pre-commit.ci

bjlittle · 2024-02-05T10:15:52Z

@tkoyama010 It's probably best to leave the author of the pull-request to merge the latest main branch changes into their branch, just incase it breaks things for them.

We can always ask them to do this at then end of a review before merging 👍

tkoyama010 · 2024-02-05T10:21:39Z

@tkoyama010 It's probably best to leave the author of the pull-request to merge the latest main branch changes into their branch, just incase it breaks things for them.

We can always ask them to do this at then end of a review before merging 👍

Thanks. I didn't notice that.

pp-mo added 5 commits January 24, 2024 20:50

Initial ideas, adding vectors to 'gv.bridge.Transform.from_points'.

bd8c868

Simplify by removing normalisation : glyph method can do this for you.

7fc4309

Move scaling options out of vectors_to_cartesian into Transform_from_…

9b6ca8e

…points.

Prevent z-scaling modifying input. Still fewer kwargs, probably.

f6823ac

Remove keywords which replicate features in .glyph()

a05ff4b

Merge branch 'main' into vector_frompoints

82155a0

pre-commit-ci bot and others added 6 commits January 27, 2024 03:42

[pre-commit.ci] auto fixes from pre-commit.com hooks

0f1ab54

for more information, see https://pre-commit.ci

Update bridge.py

58dc72a

Update common.py

5167fdb

Update bridge.py

6037a1e

Update bridge.py

48a57f3

Update bridge.py

1f1e994

bjlittle changed the title ~~Vector frompoints~~ Vector support in bridge.Transform.from_points Jan 29, 2024

stephenworsley reviewed Jan 29, 2024

View reviewed changes

tkoyama010 and others added 2 commits January 30, 2024 07:14

Merge branch 'main' into vector_frompoints

baaaee1

Add a draft example with cutdown data in a temporary location.

2ad49bb

pp-mo commented Jan 30, 2024

View reviewed changes

src/geovista/pantry/data.py Outdated Show resolved Hide resolved

Cosmetic improvements.

6e836e7

pp-mo mentioned this pull request Jan 30, 2024

Winds sample data bjlittle/geovista-data#37

Merged

stephenworsley mentioned this pull request Jan 30, 2024

Proof of concept for streamlines visualization #675

Open

Get data from newer geovista-data.

cf85be7

github-actions bot added type: examples Auto-labelled for ex/*, example/* and examples/* branches type: cache Auto-labelled labels Jan 30, 2024

pp-mo marked this pull request as ready for review January 30, 2024 17:32

pp-mo requested a review from bjlittle as a code owner January 30, 2024 17:32

pp-mo mentioned this pull request Jan 30, 2024

Proof of concept for vector visualization #674

Open

bjlittle assigned pp-mo Jan 30, 2024

Merge branch 'main' into vector_frompoints

225cfd2

[pre-commit.ci] auto fixes from pre-commit.com hooks

bc1ba77

for more information, see https://pre-commit.ci

bjlittle marked this pull request as draft February 9, 2024 22:13

HGWright assigned bjlittle and unassigned pp-mo Jun 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vector support in bridge.Transform.from_points #676

Vector support in bridge.Transform.from_points #676

pp-mo commented Jan 26, 2024 •

edited

Loading

welcome bot commented Jan 26, 2024

tkoyama010 commented Jan 27, 2024

pp-mo commented Jan 29, 2024 •

edited

Loading

stephenworsley left a comment

stephenworsley Jan 29, 2024

pp-mo Jan 30, 2024 •

edited

Loading

stephenworsley Jan 29, 2024

stephenworsley Jan 29, 2024

pp-mo Jan 30, 2024

pp-mo Jan 30, 2024

pp-mo commented Jan 30, 2024 •

edited

Loading

pp-mo commented Jan 30, 2024

pp-mo commented Jan 30, 2024

bjlittle commented Jan 30, 2024 •

edited

Loading

tkoyama010 commented Feb 5, 2024

bjlittle commented Feb 5, 2024

tkoyama010 commented Feb 5, 2024

Vector support in bridge.Transform.from_points #676

Are you sure you want to change the base?

Vector support in bridge.Transform.from_points #676

Conversation

pp-mo commented Jan 26, 2024 • edited Loading

welcome bot commented Jan 26, 2024

tkoyama010 commented Jan 27, 2024

pp-mo commented Jan 29, 2024 • edited Loading

Mini status update :

stephenworsley left a comment

Choose a reason for hiding this comment

stephenworsley Jan 29, 2024

Choose a reason for hiding this comment

pp-mo Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

stephenworsley Jan 29, 2024

Choose a reason for hiding this comment

stephenworsley Jan 29, 2024

Choose a reason for hiding this comment

pp-mo Jan 30, 2024

Choose a reason for hiding this comment

pp-mo Jan 30, 2024

Choose a reason for hiding this comment

pp-mo commented Jan 30, 2024 • edited Loading

Update :

pp-mo commented Jan 30, 2024

pp-mo commented Jan 30, 2024

bjlittle commented Jan 30, 2024 • edited Loading

tkoyama010 commented Feb 5, 2024

bjlittle commented Feb 5, 2024

tkoyama010 commented Feb 5, 2024

pp-mo commented Jan 26, 2024 •

edited

Loading

pp-mo commented Jan 29, 2024 •

edited

Loading

pp-mo Jan 30, 2024 •

edited

Loading

pp-mo commented Jan 30, 2024 •

edited

Loading

bjlittle commented Jan 30, 2024 •

edited

Loading