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

Self-contained runnable sample code for basic mesh plotting. #5501

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pp-mo
Copy link
Member

@pp-mo pp-mo commented Sep 15, 2023

Attempts to at least partially address #5500

The new sample code I believe works, and produces the provided plot.

The issue of how we stop this untested code going stale is still unresolved.

@bjlittle
Copy link
Member

bjlittle commented Sep 15, 2023

@pp-mo Adding import geovista.theme gives you better default colours IMHO.

Although, if you do this here, you'd probably want to be consistent and also do this throughout all the geovista examples in the iris docs.

Worth considering, but certainly not essential.

@codecov
Copy link

codecov bot commented Sep 15, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.37%. Comparing base (c53481e) to head (5960164).
Report is 398 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5501   +/-   ##
=======================================
  Coverage   89.37%   89.37%           
=======================================
  Files          89       89           
  Lines       22446    22446           
  Branches     5387     5387           
=======================================
  Hits        20061    20061           
  Misses       1639     1639           
  Partials      746      746           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.



# We'll re-use this to plot some real global data later.
>>> from geovista.common import to_cartesian as to_xyz
Copy link
Member

@bjlittle bjlittle Sep 15, 2023

Choose a reason for hiding this comment

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

Really?

You should adopt the use of to_cartesian and not rebrand it. The reason why you're doing this is already lost in the mists of time, and it's not recommending a good pattern to adopt to the reader moving forwards 👎

# Convert our mesh+data to a PolyData object.
# Just plotting a single height level.
>>> face_polydata = cube_faces_to_polydata(face_cube[:, 0])
...
Copy link
Member

@bjlittle bjlittle Sep 15, 2023

Choose a reason for hiding this comment

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

@pp-mo You can now simplify the call to Transform.from_unstructured as follows:

mesh = Transform.from_unstructured(
    lons.points,
    lats.points,
    connectivity=indices,
    data=cube.data,
    name=f"{cube.name()} / {cube.units}",
)

There is now no need to provide the start_index as geovista will detect this, and it's better to name the indices with the connectivity kwarg for clarity.

This is just a suggestion, and not strictly necessary.

>>> camera_pos = camera_region.mean(axis=0)
>>> my_plotter.camera.position = camera_pos

>>> my_plotter.background_color = "#502020ff"
Copy link
Member

@bjlittle bjlittle Sep 15, 2023

Choose a reason for hiding this comment

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

If you use import geovista.theme there's need to set the background_color

Comment on lines +535 to +536
>>> _ = my_plotter.add_coastlines(color="white", radius=1.01)
>>> _ = my_plotter.add_mesh(face_polydata, radius=1.0)
Copy link
Member

Choose a reason for hiding this comment

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

There's no real need here to set the radius, it's not adding anything of great value to "basic" plotting... it's just a distraction.

If you're concern is that the mesh is so low resolution that it's markedly different from the spherical coastlines, then only specify the radius for the coastlines. Setting the radius=1.0 for the face_polydata is a nop.

@bjlittle bjlittle self-requested a review September 15, 2023 20:50
Copy link
Member

@bjlittle bjlittle left a comment

Choose a reason for hiding this comment

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

@pp-mo I've raised some questions for you to consider

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

Successfully merging this pull request may close these issues.

2 participants