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

Data Structures & Helpers for Visualization #354

Merged
merged 62 commits into from
Aug 11, 2023
Merged

Conversation

philipc2
Copy link
Member

@philipc2 philipc2 commented Jun 22, 2023

Closes #171, #362, #365

Overview

  • This PR acts as a preparation for introducing visualization methods into UXarray, with new data structures and helper functions that processes the raw coordinates and connectivities stored in our Grid into a format that can be used with visualization packages (Matplotlib, Holoviews, Datashader)

New Dependencies

  • matplotlib
  • shapely
  • spatialpandas
  • antimeridian

Helper Functions & Attributes

  • Grid.polygon_shells
  • Grid.corrected_polygon_shells
  • Grid.corrected_shells_to_original_faces
  • Grid.original_to_corrected_indices
  • Grid.antimeridian_face_indices
  • grid.geometry.grid_to_polygons
  • grid.geometry._build_polygon_shells
  • grid.geometry._build_corrected_polygon_shells
  • grid.geometry._build_antimeridian_face_indices
  • grid.geometry._grid_to_polygon_geodataframe

Conversion Methods

  • Grid.to_geodataframe()
  • UxDataArray.to_geodataframe()
  • Grid.to_polycollection()
  • UxDataArray.to_polycollection()

Expected Usage

import uxarray as ux

grid_path = "/path/to/grid.nc"
data_path = "/path/to/data.nc"

uxds = ux.open_dataset(grid_path, data_path)

gdf_grid_only = uxds.uxgrid.to_geodataframe()

gdf_grid_data = uxds['v1'].to_geodataframe()

poly_grid_only = uxds.uxgrid.to_polycollection()

poly_grid_data = uxds['v1'].to_polycollection()

Visualization Examples

CSne30 (GeoDataFrame)

Grid, No Projection

image

Orthographic Projection centered around Antemeridian

image

GeoFlow (GeoDataFrame)

Grid, No Projection

image

Orthographic Projection centered around Antemeridian

image

MPAS (PolyCollection)

Shaded Polygons based on Ocean Depth, Robinson projection

image

Shaded Polygons based on Ocean Depth, Orthographic projection centered around Antemeridian

image

PR Checklist

General

  • An issue is linked created and linked
  • Add appropriate labels
  • Filled out Overview and Expected Usage (if applicable) sections

Testing

  • Adequate tests are created if there is new functionality
  • Tests cover all possible logical paths in your function
  • Tests are not too basic (such as simply calling a function and nothing else)

Documentation

  • Docstrings have been added to all new functions
  • Docstrings have updated with any function changes
  • Internal functions have a preceding underscore (_) and have been added to docs/internal_api/index.rst
  • User functions have been added to docs/user_api/index.rst

@philipc2 philipc2 linked an issue Jun 22, 2023 that may be closed by this pull request
@philipc2 philipc2 added the new feature New feature or request label Jun 22, 2023
@philipc2 philipc2 self-assigned this Jun 22, 2023
@philipc2 philipc2 changed the title DRAFT: Handling Antimeridian Faces DRAFT: Handling Antimeridian Faces & Conversion to GeoDataFrame Jun 23, 2023
@philipc2 philipc2 linked an issue Jun 28, 2023 that may be closed by this pull request
@philipc2 philipc2 changed the title DRAFT: Handling Antimeridian Faces & Conversion to GeoDataFrame DRAFT: Data Structures & Helpers for Visualization Jul 3, 2023
@philipc2 philipc2 linked an issue Jul 3, 2023 that may be closed by this pull request
@rajeeja
Copy link
Contributor

rajeeja commented Jul 28, 2023

I see this PR has cool pictures!! kudos on the good work - @ifranda and @philipc2 !

Possible to add an example notebook? - potentially something with geographic national showing some the grid and some temp/etc. data.

minor note: It'd be good to have some more documentation about the introduction of geometry.py (maybe on top of that file). Geometry means very different things in different context.

@philipc2
Copy link
Member Author

Possible to add an example notebook? - potentially something with geographic national showing some the grid and some temp/etc. data.

This is currently in the works as a separate cookbook, not directly included in our usage examples (link here). However a usage example notebook under our documentation detailing the algorithm and implementation would be good.

minor note: It'd be good to have some more documentation about the introduction of geometry.py (maybe on top of that file). Geometry means very different things in different context.

I agree, I chose the name geometry since we were using shapely to convert our unstructured grid into polygons, however I'm open to other names for the module.

@rajeeja
Copy link
Contributor

rajeeja commented Jul 29, 2023

I agree, I chose the name geometry since we were using shapely to convert our unstructured grid into polygons, however I'm open to other names for the module.

What else would this be useful for? any other geometry type operations or purely for viz purposes only? face transformations for visualisation or 2d element utils would be better.

@philipc2
Copy link
Member Author

I agree, I chose the name geometry since we were using shapely to convert our unstructured grid into polygons, however I'm open to other names for the module.

What else would this be useful for? any other geometry type operations or purely for viz purposes only? face transformations for visualisation or 2d element utils would be better.

I can see it being used to store functionality related to geometric data structures, such as KDTrees, so not necessarily visualization-only functionality.

Copy link
Member

@aaronzedwick aaronzedwick left a comment

Choose a reason for hiding this comment

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

This look great! Nice work

Copy link
Member

@erogluorhan erogluorhan left a comment

Choose a reason for hiding this comment

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

Thanks for this significant work! Please see my inline comments

ci/docs.yml Outdated Show resolved Hide resolved
ci/environment.yml Outdated Show resolved Hide resolved
docs/internal_api/index.rst Outdated Show resolved Hide resolved
docs/user_api/index.rst Show resolved Hide resolved
docs/user_api/index.rst Outdated Show resolved Hide resolved
uxarray/__init__.py Show resolved Hide resolved
uxarray/__init__.py Outdated Show resolved Hide resolved
uxarray/core/dataarray.py Outdated Show resolved Hide resolved
uxarray/grid/coordinates.py Outdated Show resolved Hide resolved
uxarray/grid/geometry.py Outdated Show resolved Hide resolved
Copy link
Member

@erogluorhan erogluorhan left a comment

Choose a reason for hiding this comment

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

Thanks for resolving a lot of things! Please see my second review's comments below

uxarray/core/dataarray.py Outdated Show resolved Hide resolved
docs/user_api/index.rst Show resolved Hide resolved
@philipc2
Copy link
Member Author

Thanks! Actually the "_grid" in each variable can get confusing; we are not caching or overriding the actual grid object but the geodataframe or polycollection that is being generated from that object. We should consider using a better name

I can rename it to cache_geometry instead.

@philipc2
Copy link
Member Author

Also, decorating several functions with property would eventually lead to a cluttered grid view in IDEs or Jupyter notebooks, which may not be ideal.

The purpose of decorating functions with properties is to allow for our Grid class to provide a better way for the user to access attributes of interest, as was the case with all of our UGRID variables (node_x, face_nodes, etc). I do see the concern in using the properties to construct the GeoDataFrame/PolyCollection, since they currently will keep the polygon shells and other variables in memory.

I need to look into it, however the memory footprint of these properties should only become an issue if there has been a call to them, since the internal attribute that is accessed by the property is initialized to None.

  @property
  def antimeridian_face_indices(self):
      """Index of each face that crosses the antimeridian."""
      if self._antimeridian_face_indices is None:
          self._antimeridian_face_indices = _build_antimeridian_face_indices(
              self)
      return self._antimeridian_face_indices

So, we could design them as helper functions that can only be called if/when the user actually needs them.

I already have these set up as helper functions, as seen above in the example. I can modify the construction code for the GeoDataFrame and PolyCollection to use the helper functions directly instead of the properties.

I'll remove these as properties for now and we can handle exposing these to the user at a later date.

@erogluorhan
Copy link
Member

I can rename it to cache_geometry instead.

Would it to be confusing to use only "cache" and "override" instead? Or maybe use one of "cache_result", or "cache_output", or "cache_gdf" and "cache_polycollection"?

@erogluorhan
Copy link
Member

Also, decorating several functions with property would eventually lead to a cluttered grid view in IDEs or Jupyter notebooks, which may not be ideal.

The purpose of decorating functions with properties is to allow for our Grid class to provide a better way for the user to access attributes of interest, as was the case with all of our UGRID variables (node_x, face_nodes, etc). I do see the concern in using the properties to construct the GeoDataFrame/PolyCollection, since they currently will keep the polygon shells and other variables in memory.

I need to look into it, however the memory footprint of these properties should only become an issue if there has been a call to them, since the internal attribute that is accessed by the property is initialized to None.

  @property
  def antimeridian_face_indices(self):
      """Index of each face that crosses the antimeridian."""
      if self._antimeridian_face_indices is None:
          self._antimeridian_face_indices = _build_antimeridian_face_indices(
              self)
      return self._antimeridian_face_indices

So, we could design them as helper functions that can only be called if/when the user actually needs them.

I already have these set up as helper functions, as seen above in the example. I can modify the construction code for the GeoDataFrame and PolyCollection to use the helper functions directly instead of the properties.

I'll remove these as properties for now and we can handle exposing these to the user at a later date.

These are all good points, and we should talk about all of these during our later discussion on this. For now, I think we should clean the following leftover from grid.py:

self._polygon_shells = None
self._corrected_polygon_shells = None
self.__corrected_shells_to_original_faces = None

and maybe other leftovers after these last minutes changes?

@philipc2
Copy link
Member Author

philipc2 commented Aug 10, 2023

I can rename it to cache_geometry instead.

Would it to be confusing to use only "cache" and "override" instead? Or maybe use one of "cache_result", or "cache_output", or "cache_gdf" and "cache_polycollection"?

The caching happens at the Grid level, not at the DataArray level, so they are stored under the Grid object. The caching is only done on the geometry (Grid), which is the most computationally expensive step.

@philipc2
Copy link
Member Author

Also, decorating several functions with property would eventually lead to a cluttered grid view in IDEs or Jupyter notebooks, which may not be ideal.

The purpose of decorating functions with properties is to allow for our Grid class to provide a better way for the user to access attributes of interest, as was the case with all of our UGRID variables (node_x, face_nodes, etc). I do see the concern in using the properties to construct the GeoDataFrame/PolyCollection, since they currently will keep the polygon shells and other variables in memory.
I need to look into it, however the memory footprint of these properties should only become an issue if there has been a call to them, since the internal attribute that is accessed by the property is initialized to None.

  @property
  def antimeridian_face_indices(self):
      """Index of each face that crosses the antimeridian."""
      if self._antimeridian_face_indices is None:
          self._antimeridian_face_indices = _build_antimeridian_face_indices(
              self)
      return self._antimeridian_face_indices

So, we could design them as helper functions that can only be called if/when the user actually needs them.

I already have these set up as helper functions, as seen above in the example. I can modify the construction code for the GeoDataFrame and PolyCollection to use the helper functions directly instead of the properties.
I'll remove these as properties for now and we can handle exposing these to the user at a later date.

These are all good points, and we should talk about all of these during our later discussion on this. For now, I think we should clean the following leftover from grid.py:

self._polygon_shells = None
self._corrected_polygon_shells = None
self.__corrected_shells_to_original_faces = None

and maybe other leftovers after these last minutes changes?

Good catch on the leftover attributes.

Copy link
Member

@erogluorhan erogluorhan left a comment

Choose a reason for hiding this comment

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

Great work! Approved but please see one inline comment below. Regardless of your response to it, feel free to merge it.

uxarray/core/dataarray.py Outdated Show resolved Hide resolved
@philipc2 philipc2 merged commit 8f0599c into main Aug 11, 2023
10 checks passed
@erogluorhan erogluorhan deleted the philipc2/wrapped-polygons branch August 11, 2023 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request
Projects
None yet
5 participants