Refactor visualization module #199

aperezhortal · 2021-01-03T14:10:10Z

Refactor the visualization module

1) Refactor visualization.utils functions

parse_proj4_string: A more robust implementation of this function is implemented in pyproj. The refactored implementation uses a direct call to the pyproj function.
proj4_to_cartopy: Refactor the proj4_to_cartopy function
- Use the parse_proj4_string to construct a dictionary with the proj definitions
- Simplify the function code by using additional dictionaries to relate the Pyproj and Cartopy parameters.
- Add support for additional cartopy keywords defined in the Proj4 definitions.
Add two plotting utility functions to avoid code duplication and to improve the axis handling:
- get_basemap_axis: This function safely gets a basemap axis. If the current axis is not a cartopy axis, it creates a basemap. Otherwise, the current axis is returned.
- get_geogrid: Returns the x,y grid used in the plots along with additional metadata.
Solves issue Call of basemaps.plot_geography clears existing cartopy axis #197.

2) Refactor plotting functions

Refactor the plotting functions to use the new plotting utils
Merge _plot_field_pcolormesh and _plot_field in a single function.
Avoid plotting the no-data mask separately. Instead, plot it in the _plot_field function.
Merge quiver and streamplots functions into a single one called plot_motion for avoiding code duplication.
The quiver and streamplot functions are now wrappers for plot_motion.
IMPORTANT
- Rename the type keyword in plot_precip_field to ptype to avoid shadowing the built-in function with the same name. Add the corresponding deprecation warning.

3) Define a unique zorder for each type of plot

Basemap features zorder
- ocean: 0
- land: 0
- lakes: 0
- rivers_lake_centerlines: 0
- coastline: 15
- cultural: 15
- reefs: 15
- minor_islands: 15
precipitation: 10
quiver: 20
stream function: 30
tracks and contours: 40

- **kwargs was replaced by the explicit keywords declarations - If unsupported kwargs are provided, they are ignored and a deprecation warning is shown.

- Use a single function to plot the quiver and the streamplots to avoid code duplication. This function is called by the quiver and streamplot functions. - Use different zorders for each type of plot - Improve the handling of existing geoaxes - Add two new utils functions to avoid code repetitions

- Fix bug in plot_precip_field handling the old type keyword.

Also, avoid plotting the no-data mask separately. Instead, plot it in the `_plot_field` function.

- Use the parse_proj4_string for construct a dictionary with the proj definitions - Simplify the function code by using additional dictionaries to relate the Pyproj and Cartopy parameters. - Add support for additional cartopy keywords defined in the Proj4 definitions.

The PYPROJ_PROJ_KWRDS_TO_CARTOPY key,values where reverted (values, keys). This produced incorrect cartopy projections that where unrelated with the proj4 definitions.

codecov · 2021-01-03T14:19:02Z

Codecov Report

Merging #199 (431d5bd) into master (6068f31) will increase coverage by 1.70%.
The diff coverage is 76.94%.

@@            Coverage Diff             @@
##           master     #199      +/-   ##
==========================================
+ Coverage   76.30%   78.01%   +1.70%     
==========================================
  Files         130      131       +1     
  Lines        9690     9638      -52     
==========================================
+ Hits         7394     7519     +125     
+ Misses       2296     2119     -177

Flag	Coverage Δ
unit_tests	`78.01% <76.94%> (+1.70%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pysteps/exceptions.py	`100.00% <ø> (ø)`
pysteps/tests/test_plt_precipfields.py	`91.17% <ø> (ø)`
pysteps/visualization/spectral.py	`16.66% <0.00%> (-0.73%)`	⬇️
pysteps/visualization/thunderstorms.py	`18.18% <14.28%> (-1.34%)`	⬇️
pysteps/visualization/basemaps.py	`71.42% <40.00%> (-7.36%)`	⬇️
pysteps/visualization/utils.py	`56.93% <75.94%> (+5.32%)`	⬆️
pysteps/visualization/animations.py	`86.72% <84.88%> (+79.98%)`	⬆️
pysteps/visualization/precipfields.py	`82.96% <89.28%> (+16.29%)`	⬆️
pysteps/visualization/motionfields.py	`92.10% <92.10%> (+21.56%)`	⬆️
pysteps/tests/test_plt_animate.py	`100.00% <100.00%> (ø)`
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6068f31...431d5bd. Read the comment docs.

pysteps/visualization/basemaps.py

pysteps/visualization/utils.py

dnerini

Well, this is really excellent work Andres and a very much needed refactoring of the visualization module! It brings a lot of clarity in the code and solves both long standing and newer issues, thanks a lot for the contribution!

I also still need to test it directly myself with some code and data (including the pysteps examples), but for the moment I went through the PR and left few minor comments.

dnerini · 2021-01-04T15:53:02Z

Basemap features zorder
    ocean: 0
    land: 0
    lakes: 0
    rivers_lake_centerlines: 0
    coastline: 2
    cultural: 2
    reefs: 2
    minor_islands: 2
precipitation: 10
quiver: 20
stream function: 30

I like the way you formalized the zorder conventions used when plotting. However, personally I like to see the national boundaries on top of the precipitation field: this allows to keep a geographical reference even when the precipitation field is very large and covers most of the domain. Hence, I'd suggest to set the coastline, cultural, reefs and minor_islands features with a zorder > 10, what do you think?

aperezhortal · 2021-01-04T16:02:01Z

I'd suggest to set the coastline, cultural, reefs and minor_islands features with a zorder > 10, what do you think?

👍 I'll make the changes.

Sets the coastline, cultural, reefs and minor_islands features with a zorder = 15. In this way, they are plotted on top of the precipitation field.

dnerini · 2021-01-11T19:40:27Z

Hi @aperezhortal, thanks for including my previous comments in this PR.

I had a look at the examples rendered from this branch, and unfortunately, it seems that something's still off, for example in thunderstorm_detection_and_tracking.py:

Figure 1, v:latest:

Figure 1, v:refactor_visualization_module:

I haven't had the chance to investigate the cause of this yet, but hopefully it relates to something minor.

- If geodata is not None but cartopy is not installed, return the x/y grids in the coordinates units defined in geogrid.

A workaround for the missing Somerc projection was already implemented in the visualization.utils module: # Note: ccrs.epsg(2056) doesn't work because the projection # limits are too strict. # We'll use the Stereographic projection as an alternative. somerc=ccrs.Stereographic,

…ctor_visualization_module

For certain situations, the quiver plots were not tight and a margin was shown

The y_grid was returned in the incorrect order for yorigin=upper

aperezhortal · 2021-01-12T03:22:22Z

Hi @dnerini, thanks for spotting this! The get_geogrid function was returning the wrong coordinates units when cartopy was not installed. I also updated the thunderstorms visualization module to deal with the geogrid=None case and remove the catch for the UnsupportedSomercProjection exception. A workaround was previously implemented and that exception was never risen.

Now the documentation is almost equal to the main branch. You may note small differences in the motion vectors since now they are plotted using the grid coordinates with the same y-origin of the data. Thus, the "steps" starts on the y-origin passed in the geodata (upper) instead of starting at the bottom as before.

dnerini · 2021-01-12T21:19:38Z

This looks really great, Andres. I noticed only one last inconsistency with the current master branch, Figure 4 in plot_steps_nowcast.py:

v:latest

v:refactor_visualization_module

Notice the reddish background color in the second image? I think the transparency for zero probabilities is not working properly.
Digging a bit into the code, I figured that cmap.set_under("white", alpha=0) was missing when plotting probabilities. It also seems that with cmap.set_under, it is not necessary to manually flag all values below the minimum threshold to obtain the desired transparency.

I implemented my suggested changes in 9a0049c. Feel free to modify or revert them as needed.

Since BoundaryNorm was used, there is no need to set the vmin, vmax explicitly.

plt.show was added to make the scripts running correctly from the command line.

aperezhortal added 12 commits January 2, 2021 09:34

Small refactoring

6618fd5

- **kwargs was replaced by the explicit keywords declarations - If unsupported kwargs are provided, they are ignored and a deprecation warning is shown.

change variable name: gl -> grid_lines

451d6d2

Update tests with new ptype keyword.

2b3f6ea

- Fix bug in plot_precip_field handling the old type keyword.

Change the step keyword order for backwards compatibility.

5d07c43

Merge _plot_field_pcolormesh and _plot_field

2eb5605

Also, avoid plotting the no-data mask separately. Instead, plot it in the `_plot_field` function.

Improve utils docstrings

4e0ab56

Change variables X,Y for x_grid, y_grid

c394b43

Use pyproj builtin functions in pyproj_proj4_string

8bbb3e6

Minor docstring edit

1bad32b

Fix reverted PYPROJ_PROJ_KWRDS_TO_CARTOPY dict

938c328

The PYPROJ_PROJ_KWRDS_TO_CARTOPY key,values where reverted (values, keys). This produced incorrect cartopy projections that where unrelated with the proj4 definitions.

aperezhortal requested a review from dnerini January 3, 2021 14:10

aperezhortal added 2 commits January 3, 2021 17:59

Fix minor code issues

83a8c41

Fix style issues and remove unused imports

031b32d

dnerini added this to the release v1.4.1 milestone Jan 4, 2021