-
Notifications
You must be signed in to change notification settings - Fork 416
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
Conversation
42c7cad
to
2fec055
Compare
2fec055
to
dbc74c0
Compare
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 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? |
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. |
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 |
1217e2a
to
daec5df
Compare
d537c74
to
c28be7a
Compare
There was a problem hiding this 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.
contour.level = 700 * units.hPa | ||
|
||
panel = MapPanel() | ||
panel.area = 'sc++' |
There was a problem hiding this comment.
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.
c28be7a
to
bc1e05a
Compare
b120cbe
to
b61753f
Compare
There was a problem hiding this 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.
src/metpy/plots/declarative.py
Outdated
try: | ||
region, modifier = re.match(r'(\w+)([-+]*)$', area).groups() | ||
except AttributeError: | ||
raise TraitError('"' + area + '" is not a valid string area.') |
There was a problem hiding this comment.
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:
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() |
There was a problem hiding this comment.
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.
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().
b61753f
to
4d2bc0b
Compare
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 ofMapPanel
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 calledzoom
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