-
Notifications
You must be signed in to change notification settings - Fork 50
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
Add Horizontal Streamfunction plots + Subpolar North Atlantic projection #898
Conversation
Hi, Can you all please review this pull request? Maybe test this functionality with other E3SM runs. I only tested with a Southern Ocean refined run: |
I changed the figsize of some of the projections (North Pacific, North Atlantic, Antarctic) to help formatting (reduce whitespace etc.) in the default config file, but I only looked at single panel plots and didn't check any of the 3 panel plots. We would need to check how the figures get formatted with the 3 panel plot |
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.
@anirban89, this is really fantastic! Here are a few comments. Some are changes I noticed on a quick look through the code. Some are notes that will need to be followed up on once I've run the full analysis on a typical cryosphere simulation so we can see the effects on our usual plots.
@@ -245,6 +245,11 @@ comparisonNorthPacificWidth = 15000. | |||
comparisonNorthPacificHeight = 5000. | |||
comparisonNorthPacificResolution = 20. | |||
|
|||
# The comparison North Atlantic grid size and resolution in km |
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.
# The comparison North Atlantic grid size and resolution in km | |
# The comparison Subpolar North Atlantic grid size and resolution in km |
mpas_analysis/default.cfg
Outdated
onePanelFigSize = (8, 7.5) | ||
onePanelFigSize = (7, 7.5) | ||
threePanelVertFigSize = (8, 22) | ||
threePanelHorizFigSize = (22, 7.5) | ||
threePanelHorizFigSize = (20, 7.5) |
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.
We will have to see what kind of an effect this has on other Antarctic plots, since it will affect all of them.
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.
comparisonGrids = ['latlon'] | ||
comparisonGrids = ['latlon', 'subpolar_north_atlantic'] |
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'm guessing this was just for testing and we don't want this by default?
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 this was for testing, you are right
mpas_analysis/default.cfg
Outdated
colormapTypeResult = indexed | ||
colormapTypeResult = continuous | ||
# the type of norm used in the colormap | ||
normTypeResult = linear | ||
# A dictionary with keywords for the norm | ||
normArgsResult = {'vmin': -12., 'vmax': 12.} | ||
# color indices into colormapName for filled contours | ||
colormapIndicesResult = numpy.array(numpy.linspace(0, 255, 38), int) | ||
# colormap levels/values for contour boundaries | ||
colorbarLevelsResult = numpy.linspace(-12., 12., 37) | ||
# colormap levels/values for ticks (defaults to same as levels) | ||
colorbarTicksResult = numpy.linspace(-12., 12., 9) | ||
|
||
# Adding contour lines to the figure | ||
contourLevelsResult = np.arange(-12., 12., 1.) | ||
contourThicknessResult = 0.5 | ||
contourColorResult = black | ||
# colormap for differences |
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.
We may want to change these but it shouldn't be done in this PR.
mpas_analysis/default.cfg
Outdated
comparisonGrids = ['latlon'] | ||
comparisonGrids = ['latlon','subpolar_north_atlantic'] |
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.
Same here, this doesn't belong in this PR.
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.
Sorry you are right
ax.set_title(title, y=1.06, **plottitle_font) | ||
ax.set_title(title,**plottitle_font) |
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.
We will have to see what the effects of this change are on other plots. It could have a positive effect on the BSF plots but be a problem for other plots.
@@ -597,7 +634,7 @@ def plot_panel(ax, title, array, colormap, norm, levels, ticks, contours, | |||
else: | |||
figsize = config.getexpression(section, 'threePanelHorizFigSize') | |||
subplots = [131, 132, 133] | |||
|
|||
aspectratio = figsize[0]/figsize[1] |
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 don't think you ended up using this so I'd remove it again.
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, we can remove this for now
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.
A few more comments.
mpas_analysis/default.cfg
Outdated
onePanelFigSize = (8, 7.5) | ||
onePanelFigSize = (7, 7.5) | ||
threePanelVertFigSize = (8, 22) | ||
threePanelHorizFigSize = (22, 7.5) | ||
threePanelHorizFigSize = (20, 7.5) |
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.
@anirban89, I think if you don't @milenaveneziani, @alicebarthel, could you have a look? We should also ask LeAnn for feedback before we merge. |
This looks great! Thanks @anirban89, @xylar. |
02acebd
to
67e972d
Compare
This worked great for me, @anirban89! |
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 understand that Anirban wants to change a few aesthetic details, but this is already great for me.
@anirban89, this is really fantastic! I'm going to post some figures where the formatting needs a little work. Let's talk about how to fix these so we can get this merged. Here's the full analysis: Here is a fake main vs. control analysis (with the same run as main and control) so we can look at the 3-panel plots: Too much whitespace between title and panel titles. Probably, just need to shorten the height of the 3-panel Antarctic figure: The title font in the Subpolar North Atlantic plots seems a lot bigger than in other plots: We don't plot North Atlantic and North Pacific regions by default but it might be worth doing a test with those regions (both 1 and 3 panel plots) to make sure your modifications here are appropriate. I'm not super worried about those until we start using them by default, though. |
@anirban89, it would be good to rebase this branch onto the latest |
Yes lets reconvene tomorrow |
We remap directly from vertices to the comparison grid.
934cc4c
to
45ee0db
Compare
@xylar I am not saying it has to be part of this PR, but I noticed that EKE is being plotted on a linear scale with units in cm^2s^-2, whereas surface speed is plotted in linear scale with units in ms^-1. I have always seen EKE being plotted on a logarithmic scale. Is there any specific reason to not have SI units for this one plot? I would expect EKE to be plotted in m^2s^-2 with a logarithmic scale and a different colormap. That will definitely make for a better visualization. Thoughts? |
A separate PR to address these things would be very welcome. The analysis was added by a summer student many years ago. It hasn't received much attention since then, in part because we don't always even include it in our output. I agree with all your suggestions: log scale, SI units and a different colormap. What colormap did you have in mind? Is there a typical choice that the community expects? |
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.
@anirban89, this is excellent work! Further tweaks can be made in follow-up PRs if we find anything that needs it but this looks like exactly what I was hoping for!
@anirban89, let's move the discussion on EKE to a separate issue. I don't want this PR to be confused with an unrelated issue. |
@lconlon, are you okay with this plot of the subpolar gyre? We'd welcome feedback. As I said, we can also tweak it to your liking later if this is okay as a first pass. (It's duplicated twice in this plot just to test model vs. model comparison. You can ignore that.) |
@anirban89, would you have time to add a documentation page for this task? |
You can make a new PR just for that. |
Summary :
Config option (
climatologyMapBSF
) for plotting climatological depth integrated transport streamfunctionocean/climatology_map_bsf.py
)arrowsOnContour<suffix>
indefault.cfg
, files updatedshared/plot/climatology_map.py
,shared/plot/colormap.py
Subpolar North Atlantic projection to visualize sub polar gyre.
shared/projection/__init__.py
,shared/climatology/comparison_descriptors.py
andshared/climatology/remap_mpas_climatology_subtask.py
shared/climatology/comparison_descriptors.py