FMS2: get_field_sizes with FMS1 emulation #111

marshallward · 2022-04-14T13:32:33Z

This patch modifies the get_field_sizes function to emulate the
FMS1 behavior when the requested sizes(:) is larger than the field's
number of dimensions.

In FMS1, field_sizes would always expect a sizes array of at least
size 4, and in the format of (nx, ny, nz, nt), and would lean on
netCDF attributes like cartesian_axes when assigning the values.

FMS2 lacks similar functionality, due its more general approach.
Previously, this was emulated in a way favorable to certain situations,
but raised issues in others.

This patch attempts to resolve the issue by using the
categorize_axes() function's ability to identify axes, either by
attributes, presumed names, or the unlimited property.

marshallward · 2022-04-14T13:37:15Z

@jiandewang can you check if this PR will fix the IAU issue?

@angus-g could you also confirm that this fixes the OBC issue? (or at least the one in mom-ocean#1531)

codecov · 2022-04-14T13:39:56Z

Codecov Report

Merging #111 (54b8ff5) into dev/gfdl (f54338a) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           dev/gfdl     #111   +/-   ##
=========================================
  Coverage     28.76%   28.76%           
=========================================
  Files           248      248           
  Lines         72967    72967           
=========================================
  Hits          20990    20990           
  Misses        51977    51977

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f54338a...54b8ff5. Read the comment docs.

jiandewang · 2022-04-15T15:04:48Z

@marshallward this fixed iau issue. I am running full UFS RT now, shall be able to give you greenlight late this afternoon. John is having trouble with hurricane regional setting, he will post related information here shortly.

JohnSteffen-NOAA · 2022-04-15T17:29:15Z

@marshallward @jiandewang I had a failed run of our regional, hat10 domain of MOM6 using the field_size_fixes branch and compiling with FMS2. I copied over my run directory along with the err.log to this location on Hera, /scratch1/NCEPDEV/hwrf/noscrub/John.Steffen/share_Marshall/OBC3_HAT10_IC_newHSK

jiandewang · 2022-04-15T17:43:40Z

/scratch1/NCEPDEV/hwrf/noscrub/John.Steffen/share_Marshall/OBC3_HAT10_IC_newHSK

I saw FATAL from PE 0: NetCDF: Variable not found in the log file. This is OBC setting which will read in a bunch of obc*nc files, this can be a clue.

jiandewang · 2022-04-15T17:44:03Z

@marshallward this fixed iau issue. I am running full UFS RT now, shall be able to give you greenlight late this afternoon. John is having trouble with hurricane regional setting, he will post related information here shortly.

update; all UFS RT runs are fine.

jiandewang · 2022-04-15T18:06:28Z

/scratch1/NCEPDEV/hwrf/noscrub/John.Steffen/share_Marshall/OBC3_HAT10_IC_newHSK

I saw FATAL from PE 0: NetCDF: Variable not found in the log file. This is OBC setting which will read in a bunch of obc*nc files, this can be a clue.

this is one of the obc file header
netcdf obc_ts_east {
dimensions:
nx_segment_003 = 1 ;
ny_segment_003 = 1267 ;
nz_segment_003_temp = 50 ;
nz_segment_003_salt = 50 ;
nz_segment_003_u = 50 ;
nz_segment_003_v = 50 ;
time = UNLIMITED ; // (213 currently)
.................
double salt_segment_003(time, nz_segment_003_salt, ny_segment_003, nx_segment_003)
............

marshallward · 2022-04-15T18:16:04Z

@jiandewang @JohnSteffen-NOAA
Thanks, I can see that one being problematic. It is probably moving the time axis to the 4th position. I haven't seen an OBC case like this one.

~~Can one of you either point me to the file (ideally on Gaea) or email it me if it's not too large?~~

marshallward · 2022-04-15T18:21:16Z

Sorry, disregard the last bit, I was able to locate the file.

JohnSteffen-NOAA · 2022-04-15T18:22:24Z

@marshallward @jiandewang Hi Marshall, if you have access to Hera, then the entire run directory and INPUT directory are there. I should note that the tests were run on Orion and I moved the directory over to Hera for accessibility. In ./INPUT, the OBC files are listed,
obc_ssh_east.nc*
obc_ssh_north.nc*
obc_ssh_south.nc*
obc_ts_east.nc*
obc_ts_north.nc*
obc_ts_south.nc*
obc_uv_east.nc*
obc_uv_north.nc*
obc_uv_south.nc*

marshallward · 2022-04-15T19:24:21Z

This is the output from field_sizes for various arrays in obc_ts_north.nc:

(Edit: I mixed up some commit, the 2d one ends in (..., 1, 1) for FMS2 with this patch, not (..., 0, 0).)

fms1:

lon_segment_001   2271           1          -1         213
temp_segment_001  2271           1          50         213
salt_segment_001  2271           1          50         213
u_segment_001     2271           1          50         213
v_segment_001     2271           1          50         213

fms2:

lon_segment_001   2271           1           1           1
temp_segment_001  2271           1          50         213
salt_segment_001  2271           1          50         213
u_segment_001     2271           1          50         213
v_segment_001     2271           1          50         213

Aside from the first one, the outputs are identical.

For the first one, the first two records are correct, and the last two differ although I would not have thought they were relevant here. But we can tweak those values if the code is expecting something different here.

Also, I should mention that this current PR has a lot of recent GFDL code changes on top of the PR candidate, but I am hoping they are not related to what you are seeing.

jiandewang · 2022-04-15T19:44:14Z

This is the output from field_sizes for various arrays in obc_ts_north.nc:
fms1:

lon_segment_001   2271           1          -1         213
temp_segment_001  2271           1          50         213
salt_segment_001  2271           1          50         213
u_segment_001     2271           1          50         213
v_segment_001     2271           1          50         213

fms2:

lon_segment_001   2271           1           0           0
temp_segment_001  2271           1          50         213
salt_segment_001  2271           1          50         213
u_segment_001     2271           1          50         213
v_segment_001     2271           1          50         213
Aside from the first one, the outputs are identical.

For the first one, the first two records are correct, and the last two differ although I would not have thought they were relevant here. But we can tweak those values if the code is expecting something different here.

Also, I should mention that this current PR has a lot of recent GFDL code changes on top of the PR candidate, but I am hoping they are not related to what you are seeing.

@marshallward: @JohnSteffen-NOAA confirmed it works fine when using infra/FMS1 code

marshallward · 2022-04-15T20:10:47Z

There was an error in my table, which I think was using the dev/gfdl version of field_size. This patch changes those padded values from 0 to 1.

@JohnSteffen-NOAA Sorry for the extra work, but could I ask that you also test the current dev/gfdl branch in your model? Just to rule out that it is not something unrelated to this PR.

JohnSteffen-NOAA · 2022-04-15T20:26:18Z

@marshallward Yes, I can test the dev/gfdl branch as a check. To be clear, this should also use FMS2, correct?

marshallward · 2022-04-15T20:28:00Z

@marshallward Yes, I can test the dev/gfdl branch as a check. To be clear, this should also use FMS2, correct?

Thats right, with the FMS2 infra.

marshallward · 2022-04-15T22:03:51Z

I looked at the backtrace, and the error is actually coming from the vertical regridding, vgrid.nc, dz. And I do replicate this error when I try to read it.

FATAL: NetCDF: Variable not found

As far as I can tell, the OBC I/O is working well enough (as long as we don't read those values of siz(:) beyond the actual size of the array).

I think that I have what I need. Thanks for providing that information, and I will try to come up with a fix.

JohnSteffen-NOAA · 2022-04-16T01:03:28Z

@marshallward Yes, I can test the dev/gfdl branch as a check. To be clear, this should also use FMS2, correct?

Thats right, with the FMS2 infra.

The dev/gfdl branch compiled with FMS2 infra passed my MOM6 test.

This patch modifies the `get_field_sizes` function to emulate the FMS1 behavior when the requested `sizes(:)` is larger than the field's number of dimensions. In FMS1, `field_sizes` would always expect a `sizes` array of at least size 4, and in the format of (nx, ny, nz, nt), and would lean on netCDF attributes like `cartesian_axes` when assigning the values. FMS2 lacks similar functionality, due its more general approach. Previously, this was emulated in a way favorable to certain situations, but raised issues in others. This patch attempts to resolve the issue by using the `categorize_axes()` function's ability to identify axes, either by attributes, presumed names, or the `unlimited` property.

marshallward · 2022-04-16T17:25:24Z

Thanks @JohnSteffen-NOAA

I've updated the PR to handle the vgrid.nc case, which does not crash and gives me consistent output. It has also passed our internal regression test.

fms1:
dz          50          -1          -1          -1
fms2:
dz          50           1           1           1

Values beyond the range of size(dz(:)) once again differ, but again I am hoping that it's not important for our runs.

I can still imagine ways to cause havoc with this function, and perhaps we should just phase it out (assuming a viable FMS1 solution can be found), but let's save that for a future discussion.

nikizadehgfdl · 2022-04-18T19:50:09Z

@marshallward thanks for this. I use this update to fix an issue with OBC tests with infra/FMS2. They would otherwise crash since field_size gets a wrong value for z-dim for ssh (3 instead of 2) on obc segments.

jiandewang · 2022-04-19T17:43:28Z

@marshallward your new commit # f6fc6ce works fine for UFS and Hurricane regional setting. (I think you did a forced push, right ?)

Hallberg-NOAA

These changes make sense to me, and it appears that they address all of our known issues. In addition to the usual pipeline and TC tests, I will be running the ocean-only and ice-ocean cases from the pipeline manually to make sure that the FMS2 interfaces are being properly exercised, but once these are done this PR should be accepted and merged.

Hallberg-NOAA · 2022-04-19T19:41:15Z

This PR has passed pipeline testing at https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/15274 and my own tests that use the FMS2 interfaces, as well as the TC testing.

update to MOM6 main 20230206 commit

marshallward force-pushed the field_size_fixes branch from 5c5c266 to f6fc6ce Compare April 16, 2022 12:08

Merge branch 'dev/gfdl' into field_size_fixes

54b8ff5

Hallberg-NOAA approved these changes Apr 19, 2022

View reviewed changes

Hallberg-NOAA merged commit b4b7368 into NOAA-GFDL:dev/gfdl Apr 19, 2022

This was referenced May 16, 2022

GFDL to main candidate branch (2022-05-16) mom-ocean/MOM6#1569

Closed

GFDL to main candidate branch (2022-05-16) mom-ocean/MOM6#1570

Merged

marshallward deleted the field_size_fixes branch June 14, 2022 19:22

marshallward mentioned this pull request Aug 1, 2022

FMS2 field size behaviour differing from FMS1 mom-ocean/MOM6#1531

Closed

marshallward pushed a commit that referenced this pull request Mar 2, 2023

Merge pull request #111 from jiandewang/feature/update-to-main-20230206

d0c4550

update to MOM6 main 20230206 commit

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FMS2: get_field_sizes with FMS1 emulation #111

FMS2: get_field_sizes with FMS1 emulation #111

marshallward commented Apr 14, 2022

marshallward commented Apr 14, 2022 •

edited

Loading

codecov bot commented Apr 14, 2022 •

edited

Loading

jiandewang commented Apr 15, 2022 •

edited

Loading

JohnSteffen-NOAA commented Apr 15, 2022

jiandewang commented Apr 15, 2022

jiandewang commented Apr 15, 2022

jiandewang commented Apr 15, 2022

marshallward commented Apr 15, 2022 •

edited

Loading

marshallward commented Apr 15, 2022

JohnSteffen-NOAA commented Apr 15, 2022

marshallward commented Apr 15, 2022 •

edited

Loading

jiandewang commented Apr 15, 2022

marshallward commented Apr 15, 2022 •

edited

Loading

JohnSteffen-NOAA commented Apr 15, 2022

marshallward commented Apr 15, 2022

marshallward commented Apr 15, 2022 •

edited

Loading

JohnSteffen-NOAA commented Apr 16, 2022

marshallward commented Apr 16, 2022

nikizadehgfdl commented Apr 18, 2022

jiandewang commented Apr 19, 2022

Hallberg-NOAA left a comment

Hallberg-NOAA commented Apr 19, 2022

FMS2: get_field_sizes with FMS1 emulation #111

FMS2: get_field_sizes with FMS1 emulation #111

Conversation

marshallward commented Apr 14, 2022

marshallward commented Apr 14, 2022 • edited Loading

codecov bot commented Apr 14, 2022 • edited Loading

Codecov Report

jiandewang commented Apr 15, 2022 • edited Loading

JohnSteffen-NOAA commented Apr 15, 2022

jiandewang commented Apr 15, 2022

jiandewang commented Apr 15, 2022

jiandewang commented Apr 15, 2022

marshallward commented Apr 15, 2022 • edited Loading

marshallward commented Apr 15, 2022

JohnSteffen-NOAA commented Apr 15, 2022

marshallward commented Apr 15, 2022 • edited Loading

jiandewang commented Apr 15, 2022

marshallward commented Apr 15, 2022 • edited Loading

JohnSteffen-NOAA commented Apr 15, 2022

marshallward commented Apr 15, 2022

marshallward commented Apr 15, 2022 • edited Loading

JohnSteffen-NOAA commented Apr 16, 2022

marshallward commented Apr 16, 2022

nikizadehgfdl commented Apr 18, 2022

jiandewang commented Apr 19, 2022

Hallberg-NOAA left a comment

Choose a reason for hiding this comment

Hallberg-NOAA commented Apr 19, 2022

marshallward commented Apr 14, 2022 •

edited

Loading

codecov bot commented Apr 14, 2022 •

edited

Loading

jiandewang commented Apr 15, 2022 •

edited

Loading

marshallward commented Apr 15, 2022 •

edited

Loading

marshallward commented Apr 15, 2022 •

edited

Loading

marshallward commented Apr 15, 2022 •

edited

Loading

marshallward commented Apr 15, 2022 •

edited

Loading