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

Switch to plotting sea-ice concentration and thickness on projection grids #915

Merged
merged 13 commits into from
Oct 8, 2022

Conversation

xylar
Copy link
Collaborator

@xylar xylar commented Sep 29, 2022

This merge moves the class PlotClimatologyMapSubtask to the shared infrastructure and generalizes it to handle both ocean and sea-ice climatology plots.

Then, it changes all existing ocean and sea-ice climatology-map tasks to use this shared task. The version of PlotClimatologyMapSubtask in sea_ice is retained because sea-ice tasks in development still need to use it.

@xylar xylar self-assigned this Sep 29, 2022
@xylar xylar force-pushed the plot_sea_ice_on_projection_grids branch 2 times, most recently from b99768d to 8ba555e Compare September 29, 2022 00:47
@xylar
Copy link
Collaborator Author

xylar commented Sep 29, 2022

@xylar
Copy link
Collaborator Author

xylar commented Sep 29, 2022

@milenaveneziani, what I'm hoping for in a review is for you to use this branch on some analysis you've already run and to compare the sea-ice plots. Hopefully, they're no worse than before and maybe even a little bit better because of the higher resolution.

@anirban89
Copy link
Collaborator

@xylar
barotropicStreamfunction_antarctic_main_ANN_years0001-0020
iceconcSH_main_DJF_years0001-0020
barotropicStreamfunction_arctic_main_ANN_years0001-0020
iceconcNH_main_JFM_years0001-0020
I just noticed that the extents of the Antarctic and Arctic projection grids are smaller than the typical comparison regions for Antarctic and Northern Hemisphere used for sea ice. Should they be the same size?

@xylar
Copy link
Collaborator Author

xylar commented Sep 29, 2022

@milenaveneziani, @anirban89 raises a really good point. Options are:

  1. increase the extent of the comparison grids to match what we've been using for sea-ice plots (makes visual comparison with analysis we already have a little harder)
  2. keep the current, smaller regions and plot sea-ice in this more limited region (probably not a good option)
  3. make separate projection grids for the Arctic and Antarctic for sea-ice and ocean (requires more mapping files)

I favor option 1. It seems like the only downsides are that we have a lot of analysis on these tighter plots and that some plots (e.g. melt-rate patterns) are hard to see on the broader grids.

@xylar xylar force-pushed the plot_sea_ice_on_projection_grids branch 2 times, most recently from 34306d8 to 146d5dd Compare September 30, 2022 20:26
@milenaveneziani
Copy link
Collaborator

@xylar, @anirban89: I personally don't think that we have to make everything consistent. Melt rates are better visualized on the smaller Antarctic map, and that's fine. With barotropic stream function my feeling is that the larger map would be better, so that we can better see the flows coming into the Arctic as well as the Antarctic Circumpolar current and associated fronts.
So, if you feel like making the map for the BSF as big as the sea-ice ones, that'd be great. But that doesn't mean that we have to plot everything on the larger maps.

@xylar
Copy link
Collaborator Author

xylar commented Sep 30, 2022

@milenaveneziani, for now, we only have one Antarctic and one Arctic projection map. They are the smaller size. We would have to add support for 2 different projection maps in order to support what you suggest.

The only reason the sea-ice plots currently have a large extent is because they aren't using a projection grid at all. They're using a lat-lon grid. But this is very wasteful and leads to coarse resolution plots. The whole point of this PR is to move away from that. But it would be an important an consequential decision to have 2 different Arctic and 2 different Antarctic projection grids (my option 3). If that's what you want, I'm fine with it. But it's not a trivial amount of work so I want to be sure.

@milenaveneziani
Copy link
Collaborator

Let's go with the larger map then, at least for now. And if cryosphere thinks that the smaller Antarctic maps are better for visualizing shelf processes, then we can add one more projection at that point.

@xylar
Copy link
Collaborator Author

xylar commented Sep 30, 2022

@milenaveneziani, I'm working on support for arctic_extended and antarctic_extended grids in addition to the ones we already have. It doesn't seem like too much work after. And I do agree with your earlier comment that the flexibility to pick between these makes sense for different analysis. I'll give this a try.

Sorry going back and forth. I thought it would be more work than it looks like it will actually be.

@xylar xylar force-pushed the plot_sea_ice_on_projection_grids branch 13 times, most recently from 3a16aad to 75b1f2c Compare October 2, 2022 22:03
@xylar
Copy link
Collaborator Author

xylar commented Oct 3, 2022

@milenaveneziani, I think this is ready for you to take a look at. It's a lot but the main thing I'd appreciate from you is, as before, making sure the analysis isn't too different from before.

Here's some SORRM results:
https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.xylar/analysis_testing/plot_sea_ice_on_projection_grids/v2.cryo.Bcase.submesoscale.ne30pg2_SOwISC12to60E2r4.chrysalis/clim_0001-0020_ts_0001-0020/

@milenaveneziani
Copy link
Collaborator

@xylar, this looks good to me. I compared your plots with the ones that Darin produced for the same simulations and I don't see anything substantially different or suspicious.
The only small change I would make is about the extended colobars: since the number of colors is limited, having extended caxis on the colorbar is not really helpful, and in fact we did not use them before.

@xylar
Copy link
Collaborator Author

xylar commented Oct 5, 2022

@milenaveneziani, that's a very good point about the extended caxis. So far, we always had an extended caxis for projection plots and never for plots from lat/lon grids, which is pretty arbitrary. The extended axis seems to be ignored when you're using a continuous colormap but it seems like for an indexed colormap, we need a new parameter for that. I'll add it.

@xylar
Copy link
Collaborator Author

xylar commented Oct 5, 2022

I don't understand it at all but it seems like my last commit broke a bunch of unrelated analysis. The only thing I can come up with to explain it is that some package must have gotten updated (likely xarray) and it caused the problem. I'll verify that by testing on develop. But this is pretty frustrating if so, since I'm careful to test with the xarray master branch every time I run the full test suite, which I do pretty often.

Hopefully, more tomorrow...

@xylar
Copy link
Collaborator Author

xylar commented Oct 5, 2022

Yep, seeing the errors on develop, too:

Computing TS for Eastern Weddell Sea Deep...
  Extracting T and S in the region...
analysis task regionalTSDiagrams_computeAntarcticRegions_EasternWeddellSeaDeep_ANN failed during run
Traceback (most recent call last):
  File "/gpfs/fs1/home/ac.xylar/mpas-work/MPAS-Analysis/develop/mpas_analysis/shared/analysis_task.py", line 322, in run
    self.run_task()
  File "/gpfs/fs1/home/ac.xylar/mpas-work/MPAS-Analysis/develop/mpas_analysis/ocean/regional_ts_diagrams.py", line 579, in run_task
    self._write_obs_t_s(self.obsDicts[obsName], zmin, zmax)
  File "/gpfs/fs1/home/ac.xylar/mpas-work/MPAS-Analysis/develop/mpas_analysis/ocean/regional_ts_diagrams.py", line 719, in _write_obs_t_s
    dsRegionMask = dsRegionMask.reset_index('nCells').drop_vars(
  File "/gpfs/fs1/home/ac.xylar/anvil/mambaforge/envs/mpas_dev/lib/python3.10/site-packages/xarray/core/dataset.py", line 5051, in drop_vars
    self._assert_all_in_dataset(names)
  File "/gpfs/fs1/home/ac.xylar/anvil/mambaforge/envs/mpas_dev/lib/python3.10/site-packages/xarray/core/dataset.py", line 5018, in _assert_all_in_dataset
    raise ValueError(
ValueError: One or more of the specified variables cannot be found in this dataset

Execution time: 0:00:05.03

Two steps forward, one step back...

@milenaveneziani
Copy link
Collaborator

I'm sorry @xylar, that's frustrating. Let me know if I can help in some way.

@xylar
Copy link
Collaborator Author

xylar commented Oct 5, 2022

Thanks @milenaveneziani. For now, I don't know enough to ask for help.

@xylar
Copy link
Collaborator Author

xylar commented Oct 6, 2022

@milenaveneziani, the problems should all be fixed in #918, so we can merge this after that one.

This is used to mask out a specified value in plots. Such a mask
value is useful for fields like sea-ice thickness that aren't
actually valid when their values are given as zero.
These are 9,000 km instead of 6,000 km in extent (by default).

Also, clean up some documentation and default projection grids
in the config files.
This is the new default to many plot functions unless a
maximum length is explicitly specified.
Use these to mask sea-ice concentration (with a default min
threshold of 0.15) and sea-ice thickness (with a minimum of zero).
Previously, all global and projection plots extended indexed
colormaps in both the min and max direction.  We don't necessarily
want that, for sea-ice variables in particular.  With this merge,
the `extend` parameter is available for modification and is set
to `"neither"` for sea-ice fields.
@xylar xylar force-pushed the plot_sea_ice_on_projection_grids branch from 2bb93d1 to 3ee7152 Compare October 6, 2022 05:18
@xylar
Copy link
Collaborator Author

xylar commented Oct 7, 2022

Copy link
Collaborator

@milenaveneziani milenaveneziani left a comment

Choose a reason for hiding this comment

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

Great. Thanks Xylar!

@xylar
Copy link
Collaborator Author

xylar commented Oct 8, 2022

Thanks @milenaveneziani!

@xylar xylar merged commit 846917e into MPAS-Dev:develop Oct 8, 2022
@xylar xylar deleted the plot_sea_ice_on_projection_grids branch October 8, 2022 05:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants