bjlittle / geovista

Cartographic rendering and mesh analytics powered by PyVista
https://geovista.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
159 stars 26 forks source link

Vector support in bridge.Transform.from_points #676

Open pp-mo opened 8 months ago

pp-mo commented 8 months ago

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:

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.

welcome[bot] commented 8 months ago

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

tkoyama010 commented 8 months ago

pre-commit.ci autofix

pp-mo commented 8 months ago

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 :

  1. in the 'from_points' call : keywords vectors + vectors_array_name and vectors_z_scaling
  2. in the glyph call, you control scaling, factor and tolerance
  3. 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.

pp-mo commented 8 months ago

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

pp-mo commented 8 months ago

Data coming here : https://github.com/bjlittle/geovista-data/pull/37

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

pp-mo commented 8 months ago

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 commented 8 months ago

@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 commented 8 months ago

pre-commit.ci autofix

bjlittle commented 8 months ago

@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 commented 8 months ago

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