Brine plume #401
Brine plume #401
Conversation
All instances of an FMS ID to the internal interpolation content is replaced with a derived type containing additional metadata recording the field's origin filename and fieldname. This additional information is required in order to replicate the axis data from the field, which is no longer provided by FMS2. The abstraction of this type also allows us to either extend it or redefine it in other frameworks as needed in the future. This primarily affects the usage of the following functions: - init_external_field - time_interp_external - horiz_interp_and_extrap_tracer The following solvers are updated: - MOM_open_boundary - MOM_ice_shelf - MOM_oda_driver - MOM_MEKE - MOM_ALE_sponge - MOM_diabatic_aux Of these, OBC was the most significant. The integer handle (fid) was previously used to determine if each segment field was constant or (if negative) read from a file. After being replaced by the derived type, a new flag was added to make this determination. All of the coupled drivers have been modified, since they support time interpolation of T and S fields. - FMS - MCT - NUOPC The NUOPC driver also includes modifications to its CFC11 and CFC12 fields. Changes to the MOM CFC modules replaces an `id == -1`-like test, which is not used by the derived type. This check has been removed, and we now solely rely on the `present(cfc_handle)` test. While this could change behavior, there does not seem to be any scenario where init_external_field would return -1 but would be passed to the function. (But I may eat these words.)
With removal of axis-based operations in FMS2 I/O, this patch removes references to these calls and replaces them with MOM `axes_info` types. References to FMS1 read into an `axistype`, but the contents are transferred to an `axis_info`. FMS2 directly populates the `axis_info` content. The `get_external_field_info` calls are modified to return `axis_info` rather than `axistype`. The redundant `get_axis_data` function is also removed from `MOM_interp_infra`, since `get_axis_info` provides an equivalent operation. Generally speaking, this is not an improvement of the codebase. The FMS1 layer does a redundant copy of data from `axistype` to `axis_info`. The FMS2 layer is significantly worse, and re-opens the file to read the axis data for each field! But if the intention is to leverage the existing API, then I don't think we have any choice at the moment. Assuming this is a relatively infrequent operation, this should not cause any measureable issues, but it needs to be watched carefully.
This patch shifts all remaining time_interp_external functions from time_interp_external to equivalent ones in time_interp_external2. Internally, time-interpolated fields are initialized with `ongrid` set to `.true.`, and such fields are assumed to be on-grid. This seems to hold for all existing instances of `time_interp_external`, but needs to be monitored in the future somehow.
The FMS1 implementation of init_external_field is case-insensitive, but the FMS2 implementation is case-sensitive, which can cause errors in older established input files. This patch sweeps through the fields of the input files and checks for a case-insensitive match (using lowercase()). This requires an additional open/close of the file.
- including now passing the dimensional scaling tests.
Tweaking the brine plume code.
- it now works better on restart
- I could move it up and make it not optional.
Codecov Report
@@ Coverage Diff @@
## dev/gfdl #401 +/- ##
============================================
- Coverage 38.17% 38.17% -0.01%
============================================
Files 269 269
Lines 76553 76586 +33
Branches 14076 14084 +8
============================================
+ Hits 29226 29234 +8
- Misses 42077 42098 +21
- Partials 5250 5254 +4
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
It failed some tests. I tried running them on chinook - the new side failed at: but the old side did not. On the other hand, the old side failed to build gen_data, then tc4 failed to find the input file: The new gen_data wasn't cleaned out by "make clean", but didn't run on the old side due to differing library versions. |
|
Does the netCDF error look like its something on our end? Or is |
|
The library does have the missing routine, it's just that the system wasn't pointing to the library. There's no -L/path/to/libdir in LDFLAGS. Also, that path in FCFLAGS is a path on new chinook, not one on old chinook. After manually building gen_data, I'm now getting a different tc4 error (but no tc2 error at all on old chinook): The input files are not in the INPUT directory for some reason, but the problem is that the temp_salt file isn't right. |
|
The autoconf ought to determine if -L is required, but perhaps chinook has found some unexpected problem. Anyway, not the primary issue here. We can talk privately if needed. |
|
This is one of those cases Marshall and I were chatting about on slack: in debug mode, the model will crash if h_bbl_drag is not well defined and CS%body_force_drag is false. |
There was a problem hiding this comment.
We have discussed this PR extensively, and I think that it can be safely merged into dev/gfdl. The new options in this PR should perhaps be considered experimental, in that there are some scenarios where issues might emerge, such as when there is wetting and drying or when the boundary layer reaches the seafloor leading to large salt fluxes being applied to thin layers, or when convective instabilities are created. However, these emerging issues can be debugged and corrected as they arise. Moreover, this new option can be completely disabled via the master switch DO_BRINE_PLUME, so even if problems do emerge, they should not impact any other runs.
|
@kshedstrom I am about to test and merge this, but I recall you saying there was an issue with some GCC compilers. Is that still the case? |
|
No, my issue is with FMS 2023.01.01. |
|
OK, great. We'll talk offline about the GFortran FMs issue. |
|
Gaea regression: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/20027 ✔️ 🟡 |
Changed a recently added OMP directive for plume_flux from private to firstprivate to reflect how this variable is actually used. This bug was introduced with PR #401, but was causing sporadic failures in some of our pipeline tests with the intel compiler (essentially due to initialized memory when openMP is used) for subsequent commits.
Feel free to squash-merge.
This is a draft of the brine plume parameterization of Nguyen et al., 2009. This shouldn't cause any trouble as is, but requires changes to SIS2 and FMScoupler before it can be turned on. It mixes the brine down from the surface to a depth given by visc%MLD. Further tweaking may occur.