Rewrite horizontal regridding to use netCDF wrapper functions #48
Conversation
This patch sets the following netCDF attributes in the function `horiz_interp_and_extrap_tracer_record` via `read_attribute`. * `missing_value` (as `_FillValue`) * `scale_factor` * `add_offset` This resolves some issues related to uninitialized values.
Set netCDF attrs in MOM_horizontal_regridding
This patch extends the `read_variable` interface to include 2d array support, in order to facilitate domainless I/O via netCDF calls. This is far from the best implementation (e.g. read_variable_2d introduces another `broadcast` alongside the original one in the horizontal regridding) but it addresses the immediate issues with `MOM_read_data()`.
Codecov Report
@@ Coverage Diff @@
## dev/gfdl #48 +/- ##
============================================
+ Coverage 28.82% 28.95% +0.12%
============================================
Files 242 242
Lines 71495 71557 +62
============================================
+ Hits 20612 20717 +105
+ Misses 50883 50840 -43
Continue to review full report at Codecov.
|
src/framework/MOM_io.F90
Outdated
| if (rc /= NF90_NOERR) call MOM_error(FATAL, hdr // trim(nf90_strerror(rc)) //& | ||
| " Difficulties reading "//trim(varname)//" from "//trim(filename)) | ||
|
|
||
| if (size(start) /= ndims .or. size(nread) /= ndims) then |
There was a problem hiding this comment.
I think that there a several problems with this test.
- Because the
startandnreadarguments are optional, there needs to be test for whether they are present to wrap the test for whether they have the right size. Otherwise, this code as written could lead to a segmentation fault. - This error handling block should probably be moved up 6 lines, so that it occurs before
startandnreadare used in the call to nf90_get_var. - The fact that this code appears to work, suggests that perhaps start and nread are not optional after all?
ndimsis a property of an input file, whereas the sizes ofstartandnreadare effectively hard coded in the call to read_variable. Could there be a case where it would be appropriate to set start and nread so that this routine could, for example, read either from a 2-d file or read a single level from a 3-d file into a 2-d array, depending on what is in the file that is being read? If so, issuing an fatal error might not be the right answer.
Could this work properly if the tests were changed to if (present(start) .and. present(nread)) then ; if ((size(start) < ndims) .or. size(nread) < ndims) then, and then the call to nf90_get_var were changed to
if (present(start) .and. present(nread) then call nf90_get_var(ncid, varid, var, start(1:ndims), nread(1:ndims)) else call nf90_get_var(ncid, varid, var) endif
There was a problem hiding this comment.
Optional arguments are pass-through, so the call to nf90_get_var does not need to be changed. But I agree with the general conclusions here.
src/framework/MOM_io.F90
Outdated
| " Difficulties reading "//trim(varname)//" from "//trim(filename)) | ||
|
|
||
| ! NOTE: We could check additional information here (type, size, ...) | ||
| rc = nf90_get_var(ncid, varid, var, start, nread) |
There was a problem hiding this comment.
Consider changing this to something like:
| rc = nf90_get_var(ncid, varid, var, start, nread) | |
| if (present(start) .and. present(nread)) then | |
| if ((size(start) < ndims) .or. (size(nread) < ndims)) call MOM_error(FATAL, & | |
| "read_variable_2d: start or nread are not large enough to read "//trim(varname)//" from "//trim(filename)) | |
| rc = nf90_get_var(ncid, varid, var, start(1:ndims), nread(1:ndims)) | |
| else | |
| rc = nf90_get_var(ncid, varid, var) | |
| endif |
There was a problem hiding this comment.
If we're going to support reading 2d slices in a 3d+ field, I would prefer stronger dimensional testing.
In truth, I don't think we should be encouraging use of this function at all. It does not support parallel I/O and problems at high resolution are inevitable.
This patch modifies read_variable_2d so that the size() tests of the optional arguments are applied before the call to nf90_get_var. The tests are also wrapped inside present() flags to avoid checking unassigned variables. Thanks to Robert Hallberg for the suggestions.
marshallward
force-pushed
the
hor_regrid
branch
from
66914df
to
9865334
Compare
|
I added the I would suggest we defer this feature until it is needed somewhere. As alluded to in the comment, this is already an inefficient and potentially unsafe function, and I don't think we want to encourage its use with additional features. Its only real purpose is backwards compatibility with explicit netCDF calls in legacy code. |
|
I believe we should be able to deprecate this function after transitioning to FMS2_io so it should be considered only for legacy support. |
|
I am perhaps less confident than others on the advisability of relying on FMS to read or write small, non-domain decomposed files if we do not have to, given our history with the FMS development team not always being as careful as we would like about respecting backward compatibility. The purpose of this commit was to consolidate all existing direct NetCDF calls within the MOM6 code into a single module, and it succeeds in doing so. With the changes that have been made, I am now more confident that we will not be generating unexplained segmentation faults or other errors. This PR has passed the TC testing, and it should be accepted (via a squash merge) once the pipeline testing has passed. When this PR is accepted, we should finally be able to close mom-ocean#1312. |
|
This has now passed pipeline testing at https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/14511. |
Hallberg-NOAA
added
enhancement
This patch replaces the explicit netCDF functions inside the horizontal remapping with generic wrapper functions.
It also resolves some issues with
axes_infocreation, and introduces a newread_variablenetCDF wrapper function.The
read_variablefunction does not support data decomposition, andThis PR was authored by @MJHarrison-GFDL with modifications by myself, and I am submitting on his behalf.