-
Notifications
You must be signed in to change notification settings - Fork 18
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
Create Basic OmF/OmA Maps #108
Conversation
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 great, just one minor comment.
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.
Maps look good. Some marks are hard to see when values are small. Suggest adding nonzero line thickness to the marker (circle) edges to make the locations of each observation more clear.
@@ -37,7 +37,7 @@ def _create_map_departures(df, domain, projection, | |||
omfscatter = MapScatter(latitude=lats, | |||
longitude=lons, | |||
data=omf) | |||
omfscatter.markersize = 1 | |||
omfscatter.markersize = 2 |
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.
Possible to add the following kwarg? omfscatter.linewidths=0.15
. I don't know API from which MapScatter is derived, but assuming it's from matplotlib and is based upon matplotlib.pyplot.scatter
that should help the markers show up a little bit better. If it doesn't work right we made need to change the edgecolor kwarg.
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.
I think this is something we will have to add to EMCPy, as I don't think all **kwargs are inherited. We probably have to add it, what do you think @kevindougherty-noaa ?
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.
Yes, I was just about to comment on this. Taking a look at MapScatter, I think it would be just as simple as adding self.linewidths
as an additional argument.
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.
I think this needs even more work in EMCPy, as I don't think the edge colors argument is passed properly, see NOAA-EMC/emcpy#80
@JacobCarley-NOAA thoughts? I think this now makes the line edges dominate the plot instead of the fill colors: |
@CoryMartin-NOAA I agree it kind of takes away from the colors. Also, this is not an issue on your end, but it looks like CONUS domain is using a 110m coastline and 50m states. Will have to figure that out in EMCPy how to handle that better. |
@CoryMartin-NOAA Yeah. You and @kevindougherty-noaa are right. I'm kind of surprised of the overwhelming effect of adding the edges to be honest. No edges is definitely preferable over this. Overall this is pretty minor. |
@kevindougherty-noaa do you approve the PR now? You requested changes earlier. |
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.
Yes, my apologies. Looks great!
Closes #96
This adds a new type of plot,
map departures
, which will plot O-F or O-A (depending on YAML) for the specified domains and variables.It computes the vmin/vmax based off of the max/min values and is always centered on 0, and also annotates the nobs, min, max on the figure.
Two examples are attached.