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

WIP: Add osnap T S climtransects #899

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

alicebarthel
Copy link

@alicebarthel alicebarthel commented Sep 23, 2022

This is to plot T,S transects at OSNAP transects, based on the obs dataset [downloaded in 2022].
I based it on woce_transects.py
A couple of questions for @milenaveneziani @xylar:
I saw that WOCE is a collection of separate transects. Currently, the obs dataset has both East and West transects in one file, is it better to split them in pre-processing to give us separate plots?
I currently only added T,S (we have no measured density).
I did not add velocities in the variables but it is in the obs dataset. I suspected the process for plotting MPAS-O velocity at transect should be separate from tracer fields. Let me know if you think otherwise.

Checklist (for @xylar) before merging:

  • copy OSNAP obs from /lcrc/group/e3sm/diagnostics/observations/Ocean/OSNAP to /lcrc/group/e3sm/public_html/diagnostics/observations/Ocean/OSNAP
  • generate README from obs.xml and copy to /lcrc/group/e3sm/public_html/diagnostics/observations/Ocean/OSNAP
  • sync OSNAP to Compy and Perlmutter

@milenaveneziani
Copy link
Collaborator

Currently, the obs dataset has both East and West transects in one file, is it better to split them in pre-processing to give us separate plots?

Yes, I would split them.

I suspected the process for plotting MPAS-O velocity at transect should be separate from tracer fields.

I'll let @xylar reply to this. My standalone script plots the mpas normalVelocity at the transect mask edges: so, no interpolation but unpleasant noisy visuals. I think Xylar has a more pleasant-to-the-eye alternative.

@xylar
Copy link
Collaborator

xylar commented Sep 24, 2022

I suspected the process for plotting MPAS-O velocity at transect should be separate from tracer fields.

I'll let @xylar reply to this. My standalone script plots the mpas normalVelocity at the transect mask edges: so, no interpolation but unpleasant noisy visuals. I think Xylar has a more pleasant-to-the-eye alternative.

Hmm, I don't think we've plotted velocities at transects except for SOSE. There, we just plot zonal, meridional or magnitude. Instead, you would want velocity normal to the transect, which would require a computation based on the zonal and meridional components, and the angle of the transect edge with respect to east. I think that would be fun to do but it hasn't been done yet.

@milenaveneziani
Copy link
Collaborator

We actually don't plot velocities for the SOSE transects, only for the SOSE 2d maps.
But yes, I agree that plotting velocities is something that will take more effort and maybe should be done later.

@xylar
Copy link
Collaborator

xylar commented Sep 25, 2022

We actually don't plot velocities for the SOSE transects, only for the SOSE 2d maps.

At least in the analysis I'm looking at, we do plot the velocities (zonal, meridional and magnitude):
https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.xylar/analysis_testing/anvil/fix_conda_mpi_on_hpc/main_py3.9/ocean/index.html#sose_transects

Comment on lines 2598 to 2603
# Horizontal bounds of the plot (in km), or an empty list for automatic bounds
# The bounds are a 2-element list of the minimum and maximum distance along the
# transect. Note: A21=Drake Passage; A23=South Atlantic; A12=Prime Meridian
horizontalBounds = {'WOCE_A21': [],
'WOCE_A23': [],
'WOCE_A12': []}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can just get rid of this. The horizontal bounds can be the whole transect unless you think you would ever want to plot part of it.

@xylar xylar added this to In progress in v1.8.0 via automation Oct 13, 2022
@xylar xylar self-assigned this Oct 13, 2022
@xylar xylar removed this from In progress in v1.8.0 Nov 24, 2022
@alicebarthel
Copy link
Author

alicebarthel commented Dec 6, 2022

An update on this:

  1. I have been able to run the osnapTransects for all 3 combinations of grids (mpas, obs, and 5+uniform).
  2. I am still finding some inconsistencies.

At the moment, runnning the analysis in a new directory fails with

srun: error: b577: tasks 1,10: Exited with exit code 1
Traceback (most recent call last):
  File "/gpfs/fs1/home/ac.abarthel/mpas-analysis/add_OSNAP_TSclimtransects/mpas_analysis/__main__.py", line 381, in add_task_and_subtasks
    analysisTask.setup_and_check()
  File "/gpfs/fs1/home/ac.abarthel/mpas-analysis/add_OSNAP_TSclimtransects/mpas_analysis/ocean/compute_transects_subtask.py", line 221, in setup_and_check
    super().setup_and_check()
  File "/gpfs/fs1/home/ac.abarthel/mpas-analysis/add_OSNAP_TSclimtransects/mpas_analysis/shared/climatology/remap_mpas_climatology_subtask.py", line 208, in setup_and_check
    self._setup_remappers()
  File "/gpfs/fs1/home/ac.abarthel/mpas-analysis/add_OSNAP_TSclimtransects/mpas_analysis/shared/climatology/remap_mpas_climatology_subtask.py", line 413, in _setup_remappers
    self.remappers[comparisonGridName] = get_remapper(
  File "/gpfs/fs1/home/ac.abarthel/mpas-analysis/add_OSNAP_TSclimtransects/mpas_analysis/shared/climatology/climatology.py", line 130, in get_remapper
    remapper.build_mapping_file(method=method, logger=logger,
  File "/lcrc/group/e3sm/ac.abarthel/mambaforge/envs/mpas_dev/lib/python3.10/site-packages/pyremap/remapper.py", line 303, in build_mapping_file
    subprocess.check_call(args, stdout=DEVNULL)
  File "/lcrc/group/e3sm/ac.abarthel/mambaforge/envs/mpas_dev/lib/python3.10/subprocess.py", line 369, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['srun', '-n', '12', '/lcrc/group/e3sm/ac.abarthel/mambaforge/envs/mpas_dev/bin/ESMF_RegridWeightGen', '--source', '/lcrc/group/e3sm/ac.abarthel/analysis/debugl.A_WCYCL1850.ne4_oQU480.anvil/clim_3-5_ts_1-5/mapping/tmptykhhv_f/src_mesh.nc', '--destination', '/lcrc/group/e3sm/ac.abarthel/analysis/debugl.A_WCYCL1850.ne4_oQU480.anvil/clim_3-5_ts_1-5/mapping/tmptykhhv_f/dst_mesh.nc', '--weight', '/lcrc/group/e3sm/ac.abarthel/analysis/debugl.A_WCYCL1850.ne4_oQU480.anvil/clim_3-5_ts_1-5/mapping/map_oQU480_to_OSNAP_transects_5km_bilinear.nc', '--method', 'bilinear', '--netcdf4', '--no_log', '--src_loc', 'center', '--src_regional', '--dst_regional', '--ignore_unmapped']' returned non-zero exit status 1.
Warning: osnapTransects: remapTransects failed during check and will not be run
Warning: subtask of osnapTransects failed during check, so this task will not be run

but re-running it a second time (without purging) works and generates the plots. I am wondering if there is something in the order of operations that is off. After the first failure, the only logs available are:

ls /lcrc/group/e3sm/ac.abarthel/analysis/debugl.A_WCYCL1850.ne4_oQU480.anvil/clim_3-5_ts_1-5/logs
config.A_WCYCL1850.ne4_oQU480.anvil  taskProgress.log

@xylar
Copy link
Collaborator

xylar commented Dec 6, 2022

@alicebarthel, you're getting close! Because you are using a development environment that doesn't support system MPI, you need these config options from the tutorial: https://mpas-dev.github.io/MPAS-Analysis/latest/tutorials/dev_getting_started.html#execute

That way, ESMF_RegridWeightGen won't be called with srun.

@alicebarthel
Copy link
Author

Thanks @xylar ! It seems to work ok now that I added the [execute] section. Apologies for rushing through the set-up instead of reading the tutorial carefully. Just curious: Why does running it a second time solve the problem?

@alicebarthel
Copy link
Author

A couple of questions/comments:

  • The default grid for WOCE transects is: (horiz: 5, vert: uniform_0_to_4000m_at_10m). Do we want the same for the OSNAP transects or would something else make more sense?
  • I plan to try to run the OSNAP analysis on a simulation with finer mesh.
  • Do any of you want to try to run this analysis independently to do a check on it?

@xylar
Copy link
Collaborator

xylar commented Dec 6, 2022

Just curious: Why does running it a second time solve the problem?

I have no idea. I wouldn't expect that to be the case.

@xylar
Copy link
Collaborator

xylar commented Dec 6, 2022

The default grid for WOCE transects is: (horiz: 5, vert: uniform_0_to_4000m_at_10m). Do we want the same for the OSNAP transects or would something else make more sense?

Maybe too deep?

Do any of you want to try to run this analysis independently to do a check on it?

Yes, I'll run a test, too, if you think it's ready.

@xylar
Copy link
Collaborator

xylar commented Dec 6, 2022

Apologies for rushing through the set-up instead of reading the tutorial carefully.

Absolutely no worries. It's confusing and I hope we can move to the approach in compass soon, where ESMF is built with system MPI.

@milenaveneziani
Copy link
Collaborator

The default grid for WOCE transects is: (horiz: 5, vert: uniform_0_to_4000m_at_10m). Do we want the same for the OSNAP transects or would something else make more sense?

I actually think this is OK, especially for OSNAP West. Here are two sections I plotted in the past, and they go to 3500 and 4000 m, respectively:

Temp_OSNAPsectionEast_noiceIC_SSSrest_ANN_years0001-0005

Temp_OSNAPsectionWest_noiceIC_SSSrest_ANN_years0001-0005

I can also give this a try.

@alicebarthel
Copy link
Author

Ok, thanks @milenaveneziani! I may need to adjust the plot options like range. I did not see observed densities. Did you recalculate these or did you have them?

@alicebarthel
Copy link
Author

@xylar
Copy link
Collaborator

xylar commented Dec 7, 2022

This is really great, @alicebarthel!

Comment on lines +208 to +210
analyses.append(ocean.OsnapTransects(config, oceanClimatolgyTasks['avg'],
controlConfig))

Copy link
Collaborator

Choose a reason for hiding this comment

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

You'll need to rebase onto the current develop when you get a chance. After doing that, it will be important to fix this typo that got fixed elsewhere in this file.

Suggested change
analyses.append(ocean.OsnapTransects(config, oceanClimatolgyTasks['avg'],
controlConfig))
analyses.append(ocean.OsnapTransects(config, oceanClimatologyTasks['avg'],
controlConfig))

Copy link
Collaborator

@xylar xylar left a comment

Choose a reason for hiding this comment

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

Things are looking great!

I ran the test suite and results are here:
https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.xasay-davis/analysis_testing/chrysalis/add_OSNAP_TSclimtransects/

In addition to a couple of very small code changes, the only thing left (other than the check-list at the top of this PR for me) are:


from mpas_analysis.shared.io.utility import build_obs_path

from collections import OrderedDict
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
from collections import OrderedDict

Let's take this out. I'm sure it was copied from elsewhere in MPAS-Analysis but it's an old concept from pyhton 2.7. In python 3, the default dict is ordered (whereas in python 2, it could end up being in random order).

Copy link
Collaborator

Choose a reason for hiding this comment

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

It doesn't look like it's actually used anyway.

The task that produced the climatology to be remapped and plotted
as a transect

controlconfig : mpas_tools.config.MpasConfigParser, optional
Copy link
Collaborator

Choose a reason for hiding this comment

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

Incorrect capitalization (no doubt my fault, copied from elsewhere).

Suggested change
controlconfig : mpas_tools.config.MpasConfigParser, optional
controlConfig : mpas_tools.config.MpasConfigParser, optional

"""
# Authors
# -------
# Xylar Asay-Davis
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Xylar Asay-Davis
# Alice Barthel

ocean
</component>
<description>
OSNAP,designed to provide a continuous record of the full-water column, trans-basin fluxes of heat, mass and freshwater in the subpolar North Atlantic,consists oftwo legs: one extending from southern Labrador to the southwestern tip of Greenland across the mouth of the Labrador Sea (OSNAP West), and the second from the southeastern tip of Greenland to Scotland (OSNAP East). The observing system also includes subsurface floats (OSNAP Floats) in order to trace the pathways of overflow waters in the basin and to assess the connectivity of currents crossing the OSNAP line. The location of the OSNAP East and West legs purposefully melds with a number of long-term observational efforts in the North Atlantic: the Canadian repeat AR7W program in the Labrador Sea; the German Labrador Sea western boundary array at 53°N; the global Ocean Observatories Initiative node to be placed in the southwestern Irminger Sea; the repeat A1E/AR7E hydrographic sections across the Irminger and Iceland basins; and the Ellett line in the Rockall region. Importantly, this observing system, in conjunction with the RAPID/MOCHA array at 26ºNand the EU THOR/NACLIM program, will provide a comprehensive measure of the Atlantic Meridional Overturning Circulation (AMOC) and provide a means to evaluate intergyre connectivity in the North Atlantic. OSNAP is a collaborative effort, which includes several countries including US, Canada, China, France, Germany, Netherlands and the UK.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you split this across several lines for simplicity? There seems to be a Nand that should be N and.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please take a look at:
https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.xasay-davis/analysis_testing/chrysalis/add_OSNAP_TSclimtransects/docs/users_guide/obs/osnap_t_s.html#osnap-t-s
and make sure the resulting output looks like it should. It looks good to me other than the missing space I pointed out above.

@xylar
Copy link
Collaborator

xylar commented Dec 7, 2022

I ran analysis on CRYO1950.ne30pg2_SOwISC12to60E2r4.N2Dependent.smep and it worked great:
https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.xylar/analysis/20221116.CRYO1950.ne30pg2_SOwISC12to60E2r4.N2Dependent.submeso.chrysalis/yrs121-130/ocean/index.html
It seems like we either have giant temperature biases in that run (very believable) or the bias plot has a pretty narrow range. What do you both think?

@milenaveneziani
Copy link
Collaborator

hmm, a bias of 2deg is pretty high in those regions.. We could extend the range to +-3 for the bias.

Do you understand why the range above 0 (in the vertical) is plotted?

@alicebarthel : no, I computed the sigma0's myself. They were not in the obs data set I used.

@milenaveneziani
Copy link
Collaborator

But I agree this is looking great. Thanks @alicebarthel!

@xylar
Copy link
Collaborator

xylar commented Dec 7, 2022

Do you understand why the range above 0 (in the vertical) is plotted?

@milenaveneziani, I think that's because it plots up to the actual sea surface height, which goes at least slightly above zero. It doesn't bound the data range too tightly but instead leaves a little margin of error around it. We can manually set the vertical range in the config options if this bugs you.
https://github.com/alicebarthel/MPAS-Analysis/blob/add_OSNAP_TSclimtransects/mpas_analysis/default.cfg#L2619

@milenaveneziani
Copy link
Collaborator

milenaveneziani commented Dec 7, 2022

yes, I would probably add vertical_bounds. Maybe 0 to -4000?
Thanks for reminding me about this option @xylar.

@milenaveneziani
Copy link
Collaborator

@alicebarthel: I think you can remove the WIP in the title and work in progress label for this one :)

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.

None yet

3 participants