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

Switch spherical viz to use uxarray and holoviews #140

Merged
merged 10 commits into from
Oct 27, 2023

Conversation

xylar
Copy link
Collaborator

@xylar xylar commented Oct 20, 2023

This merge introduces several new dependencies used for plotting:

  • antimeridian
  • geoviews
  • holoviews
  • hvplot
  • spatialpandas
  • uxarray

The approach used here is loosely based on the tutorials from uxarray provided here:
https://uxarray.readthedocs.io/en/latest/examples/005-to-geodataframe-for-holoviz.html
and here:
https://uxarray.readthedocs.io/en/latest/examples/004-working-with-mpas-grids.html

The MPAS mesh and an associated data array are converted to a SpatialPandas GeoDataFrame for visualization in holoviews using its matplotlib backend. (The alternative bokeh backend is nice for working in
Jupyter notebooks within a web browser but I did not find it useful for exporting images to files.)

At least for the mesh sizes and types of visualization we are currently doing on the sphere in Polaris, this new approach is much faster than generating mapping files, remapping the data to a regular grid, and then visualizing.

The to_geodataframe() method from uxarray currently only works reliably for DataArrays on cells, not on edges or vertices. Because of this, I have dropped plots of the normal velocity on edges from the geostrophic tasks. We can re-introduce this later if desired but it seems like a lot of work for some pretty noisy viz.

Checklist

  • User's Guide has been updated
  • Developer's Guide has been updated
  • API documentation in the Developer's Guide (api.md) has any new or modified class, method and/or functions listed
  • Documentation has been built locally and changes look as expected
  • Testing comment in the PR documents testing used to verify the changes

@xylar xylar added enhancement New feature or request framework Changes relating to the polaris framework as opposed to individual tests or analysis ocean Related to ocean tests or analysis labels Oct 20, 2023
@xylar xylar requested review from sbrus89 and cbegeman October 20, 2023 12:06
@xylar xylar self-assigned this Oct 20, 2023
@xylar
Copy link
Collaborator Author

xylar commented Oct 20, 2023

@cbegeman and @sbrus89, I could use your eyes on this when you have a chance. I am currently testing all the affected tasks on Chrysalis and can point you to a tarball with all the images once my testing is done.

So far, the approach seems better. It's faster, though not lightning fast. It has a few quirks like when it decides have a flat vs. pointy end on the colorbar. I'm sure we can manually interfere but I didn't want to take the trouble for now.

Comment on lines +110 to +114
### inertial_gravity_wave

```{eval-rst}
.. currentmodule:: polaris.ocean.tasks.inertial_gravity_wave

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 think we want to keep these headings alphabetized here in the API so they mimic the directory structure in the Polaris package.


# spherical: please keep these in alphabetical order
add_cosine_bell_tasks(component=self)
add_geostrophic_tasks(component=self)
add_sphere_transport_tasks(component=self)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We want to organize these tasks roughly alphabetically first by planar, single_column, spherical and then by specific subdirectory. Since this isn't easy to do with the 4 types of tasks in spherical transport, we'll put it at the end.

@xylar xylar force-pushed the switch-spherical-viz-to-uxarray branch 3 times, most recently from 06f9182 to c05eccd Compare October 20, 2023 15:46
@xylar
Copy link
Collaborator Author

xylar commented Oct 20, 2023

Testing

I was able to run all with_viz test cases on Chrysalis. The images that result are tarred up in:

/lcrc/group/e3sm/ac.xylar/polaris_0.2/chrysalis/test_20231020/spherical-convergence-uxarray-viz/images.tar.gz

@xylar
Copy link
Collaborator Author

xylar commented Oct 20, 2023

I'm seeing some glitches along the antimeridian that I may need to work with the uxarray folks on fixing:
init

Copy link
Collaborator

@cbegeman cbegeman left a comment

Choose a reason for hiding this comment

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

@xylar This looks great to me! I ran successfully on perlmutter with gnu, mpich. I also took a look at your tarred images and compared them with previous figs and they look good to me. Yes, it would be nice to have arrows on the colorbar but I agree that it doesn't need to hold us up.

@xylar xylar force-pushed the switch-spherical-viz-to-uxarray branch from c05eccd to 5c41a86 Compare October 24, 2023 11:30
Extend colorbar at both limits and make it skinnier.
@xylar xylar force-pushed the switch-spherical-viz-to-uxarray branch from fcdbe82 to da32a1f Compare October 24, 2023 16:15
@xylar
Copy link
Collaborator Author

xylar commented Oct 24, 2023

I seem to have fixed the last 2 aesthetic issues with the colorbar:

final

Comment on lines +112 to +114
def plot_global_lat_lon_field(lon, lat, data_array, out_filename, config,
colormap_section, title=None, plot_land=True,
colorbar_label=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

@xylar - I think it's good to leave in the existing capability to plot on a remapped lat/lon grid. I'm just wondering if the holowview approach is efficient enough to replace it more generally, i.e. for MPAS-Analysis purposes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It might be. @cbegeman and I were talking about this. We currently do analysis on a comparison grid and that sometimes makes it easier to compare with observations or with other model runs on a different mesh. But there's not really a reason we couldn't do as much as possible on the native mesh.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point about using a comparison grid for in those cases. That makes sense. I was just curious.

@@ -2,17 +2,116 @@

import cartopy
import cmocean # noqa: F401
import geoviews.feature as gf
import holoviews as hv
import hvplot.pandas # noqa: F401
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the use for this import?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The short answer is that I'm following this example:
https://uxarray.readthedocs.io/en/latest/examples/005-to-geodataframe-for-holoviz.html
I believe the longer answer is that this import is what makes the later call to hvplot.polygons() on the GeoDataFrame possible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Was there something you wanted me to do here or were you just curious?

Copy link
Contributor

@sbrus89 sbrus89 Oct 27, 2023

Choose a reason for hiding this comment

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

Thanks for the info. I was just curious since it is an unused import.

def plot_global_field(lon, lat, data_array, out_filename, config,
colormap_section, title=None, plot_land=True,
colorbar_label=None):
def plot_global_mpas_field(mesh_filename, da, out_filename, config,
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be of interest to add an option to add the cell outline to the plot?

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 could see that being useful and we do that with the planar plots.

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 added this capability. However, it exposed two less-than-ideal issues:

  • There doesn't seem to be a way to set the color of the edges -- they are blue no matter what color I tell it to use. I didn't find a workaround.
  • There seems to be some kind of glitch in the handling of the antimeridian, which is much more visible when plotting with edges. I will see if this problem persists in a future release of uxarray and follow up with them if it does.
    final

@xylar
Copy link
Collaborator Author

xylar commented Oct 27, 2023

@sbrus89, could you let me know if you are happy with the handling of edge plotting, given the limitations I listed above? Is there anything else you'd like to see here?

Copy link
Contributor

@sbrus89 sbrus89 left a comment

Choose a reason for hiding this comment

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

@xylar - This is a great new feature to have! I tested all the with_viz cases on Anvil and other than the dateline artifacts you already mentioned, everything worked as expected. Thanks for adding the edge feature. I think it's fine to leave it as-is despite the current drawbacks. It seems like those are things that can/will be addressed in a future uxarray release.

@xylar
Copy link
Collaborator Author

xylar commented Oct 27, 2023

@sbrus89 and @cbegeman, thank so much for you reviews! I really appreciate them!

@xylar xylar merged commit ea309c8 into E3SM-Project:main Oct 27, 2023
@xylar xylar deleted the switch-spherical-viz-to-uxarray branch October 27, 2023 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request framework Changes relating to the polaris framework as opposed to individual tests or analysis ocean Related to ocean tests or analysis
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants