*Fix indexing bugs in get_field_nc #334
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixed two horizontal indexing bugs in get_field_nc, where the difference between the array starting index (always 1 in this subroutine) and the values in the handle argument were not being taken into account when the array was being passed in with only its computational domain. Also initialized the internal unlim_index array in get_netcdf_fields to fix a problem with using an uninitialized array that was being flagged when run in debug mode. With this commit, the model is once again reproducing the expected answers when rescaling is applied for vertical distances or time. All answers are bitwise identical in cases that were working before. This commit addresses the issue at github.com/NOAA-GFDL/issues/333, which can be closed once this commit is merged into dev/gfdl.
Hallberg-NOAA
added
bug
Codecov Report
@@ Coverage Diff @@
## dev/gfdl #334 +/- ##
=============================================
+ Coverage 37.16% 47.58% +10.41%
=============================================
Files 265 41 -224
Lines 74508 4535 -69973
Branches 13839 797 -13042
=============================================
- Hits 27692 2158 -25534
+ Misses 41722 2197 -39525
+ Partials 5094 180 -4914 see 234 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
This patch addresses some of the proposed changes to `get_field_nc`. * `WARNING` is removed from `use MOM_error_handler` since it is unused. * Zero initialization of `values_c` by `source=` has been removed. This initialization serves no purpose, since every value will be filled by the netCDF read statement. * `values_c` indexing is now 1-based `values` has ambiguous shape, so it can follow no natural indexing. Although `values_c` always resides on the compute domain, the transfer to `values` may feel unintuitive. Allocating from the 1 index identifies this as an explicit transfer of 1-based indices. * Computation of the data domain indices were moved to the actual transfer of `values_c` to `values`. * A comment was added to explain the initial "magic value" of `unlim_index`. (It has also been suggested that the zero initialization of the intent(out) `values` array should be removed, even when it has halos that would not be set here. However, this was deemed beyond the scope of this bug-fix PR, so this commit was edited during merging to restore the initialization of `values` to exactly what was there on the dev/gfdl branch before this PR, leaving this particular change to be addressed with a later PR as necessary. - R. Hallberg) Stylistic changes consistent with the existing file have also been imposed: * `values_nc` renamed to `values_c` Previous suffix indicated some special netCDF property of this field, when it's just the compute subdomain of the field's values. * Comments have been truncated for readability * Column width is restricted to 80 * Composite statements (semicolons) have been removed
Hallberg-NOAA
force-pushed
the
get_field_nc_indexing_bugfix
branch
from
7f9b66e
to
e0fcbc7
Compare
Made the values argument to MOM_read_data_2d and get_field_nc intent inout to preserve the previous values of this array in any halos it might have, and removed a line initializing the halos of this array to 0. Also added some missing comments describing arguments to these routines and changed the existing comments describing the arguments to get_field_nc to use doxygen syntax. All answers in test cases are bitwise identical.
|
I have now changed the values arguments to |
|
cannot comment much on the code (LGTM) but this fix is necessary to run OM4 with debug compiler flags or else it dies trying to read the tidal amplitude file. |
marshallward
approved these changes
Mar 15, 2023
There was a problem hiding this comment.
Not a fan of the comment bloat in daea32f but you've worn me down. I approve.
|
Gaea regression: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/18626 ✔️ |
This was referenced Apr 6, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixed two horizontal indexing bugs in
get_field_nc(), where the difference between the array starting index (always 1 in this subroutine) and the values in the handle argument were not being taken into account when the array was being passed in with only its computational domain. Also initialized the internalunlim_indexarray inget_netcdf_fields()to fix a problem with using an uninitialized array that was being flagged when run in debug mode. With this commit, the model is once again reproducing the expected answers when rescaling is applied for vertical distances or time. All answers are bitwise identical in cases that were working before.This commit addresses the issue at github.com//issues/333, which can be closed once this commit is merged into dev/gfdl.