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

Region modifiers with '+' or '-' suffix #1907

Merged
merged 2 commits into from Jun 18, 2021

Conversation

23ccozad
Copy link
Contributor

@23ccozad 23ccozad commented Jun 9, 2021

In GEMPAK, users have the ability to add '+' or '-' to the end of a string given to the GAREA parameter, which zooms in or out of the specified region. This PR makes this functionality possible for the area attribute of MapPanel

Description Of Changes

Checks for presence of '+' or '-' in area strings when drawing map. The number of pluses and minuses are counted. A plus is worth 1, and a minus is worth -1. The sum is called zoom and is passed to _zoom_extent.

_zoom_extent is a new method that takes an extent and expands or contracts it. This method isn't always perfect in that it does not take into account the projection. It works well for plate carree, but it's a bit off with the more distorted projections. See test_declarative_region_modifier_zoom_out.png for an example.

Additionally, two new tests are introduced: one for zooming in and another for zooming out.

Checklist

@23ccozad 23ccozad force-pushed the region_modifiers branch 4 times, most recently from 42c7cad to 2fec055 Compare June 9, 2021 18:54
@23ccozad 23ccozad marked this pull request as ready for review June 9, 2021 19:34
@23ccozad 23ccozad added Area: Plots Pertains to producing plots Type: Feature New functionality labels Jun 9, 2021
@23ccozad 23ccozad added this to the 1.1.0 milestone Jun 9, 2021
@kgoebber
Copy link
Collaborator

Great PR! Took it for a quick spin and works great with some pre-existing scripts that use the declarative syntax.

The only thing I'm wondering, which is related, but not necessarily tied to this exact PR, is if we should publish all of the different valid area strings. I know all of these areas were ported from GEMPAK, but even then they were hard to find there. It might be useful to have a link within the doc-string for the area attribute that has the list of names and lat/lon extents as there are potentially many useful ones we have defined, but that you would be hard pressed to know about. I wouldn't want to just list them as part of the doc-string because there are a lot and that would just overwhelm the page.

If we do want to go the route of a separate page, would it be worth having a small static image associated with each area to better illustrate the extent?

@dopplershift
Copy link
Member

This looks pretty good. One thing jumps out, though: I wonder if this might work better if we did it through traitlets validation. This would allow us to give users a good error when they try to set the extent rather than failing at draw time.

If this works out, it might show us a path forward on validating some more traits.

@23ccozad
Copy link
Contributor Author

The only thing I'm wondering, which is related, but not necessarily tied to this exact PR, is if we should publish all of the different valid area strings.

The docs say "For a CONUS region, the following strings can be used: ‘us’, ‘spcus’, ‘ncus’, and ‘afus’. For regional plots, US postal state abbreviations can be used." But now having seen declarative.py and knowing that there are over 400 valid strings that can be used, I've had the same thoughts. If there's nothing in the docs about them, they're probably not being used. I'll be thinking about how we can add this to the docs, hopefully in a simple way.

@23ccozad 23ccozad force-pushed the region_modifiers branch 4 times, most recently from 1217e2a to daec5df Compare June 11, 2021 22:13
@23ccozad 23ccozad force-pushed the region_modifiers branch 2 times, most recently from d537c74 to c28be7a Compare June 14, 2021 15:44
Copy link
Member

@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

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

Looks like we're on a good path here, I just found some things that I think can simplify it.

src/metpy/plots/declarative.py Outdated Show resolved Hide resolved
src/metpy/plots/declarative.py Outdated Show resolved Hide resolved
src/metpy/plots/declarative.py Outdated Show resolved Hide resolved
src/metpy/plots/declarative.py Outdated Show resolved Hide resolved
contour.level = 700 * units.hPa

panel = MapPanel()
panel.area = 'sc++'
Copy link
Member

Choose a reason for hiding this comment

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

Geez, that's twice zoomed in? I'll have to look at original vs. 1 zoom.

@23ccozad 23ccozad force-pushed the region_modifiers branch 2 times, most recently from b120cbe to b61753f Compare June 17, 2021 14:58
Copy link
Member

@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

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

Found a few other stylistic changes that seem to clean things up. Especially using f-strings in the exceptions.

Comment on lines 670 to 673
try:
region, modifier = re.match(r'(\w+)([-+]*)$', area).groups()
except AttributeError:
raise TraitError('"' + area + '" is not a valid string area.')
Copy link
Member

Choose a reason for hiding this comment

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

Technically this try...except would catch any uncaught AttributeError happening anywhere while executing that line. While this definitely works, and a weird AttributeError bubbling up is unlikely, I feel like explicitly checking for None (which is what is documented as the behavior for "no match" in the docs) would be better style:

Suggested change
try:
region, modifier = re.match(r'(\w+)([-+]*)$', area).groups()
except AttributeError:
raise TraitError('"' + area + '" is not a valid string area.')
match = re.match(r'(\w+)([-+]*)$', area)
if match is None:
raise TraitError(f'"{area}" is not a valid string area.')
region, modifier = match.groups()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, didn't think about other errors coming up.

src/metpy/plots/declarative.py Outdated Show resolved Hide resolved
src/metpy/plots/declarative.py Outdated Show resolved Hide resolved
src/metpy/plots/declarative.py Outdated Show resolved Hide resolved
TraitError thrown when bad inputs are provided to 'area' trait. Code to determine map extent using 'area' string is removed from draw() and included in the validation method, _valid_area().
@dopplershift dopplershift merged commit 273a72a into Unidata:main Jun 18, 2021
@23ccozad 23ccozad deleted the region_modifiers branch June 28, 2021 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Plots Pertains to producing plots Type: Feature New functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GEMPAK-style region modifiers Declarative extent zooming in/out
3 participants