IO: get_field_nc values to intent(inout)#9
Closed
marshallward wants to merge 2 commits into
Closed
Conversation
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` has been removed I find no evidence that `MOM_read_data` or its principal function call, `mpp_read`, apply any initialization to the halo data. Also, there is no reason why the user should expect a `read()`-like function to populate these fields, so this has been removed. If a field needs these initialized, then it should be done outside of the `read()` call. * 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`. 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
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## get_field_nc_indexing_bugfix #9 +/- ##
===============================================================
Coverage ? 37.15%
===============================================================
Files ? 265
Lines ? 74516
Branches ? 13839
===============================================================
Hits ? 27690
Misses ? 41731
Partials ? 5095 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Hallberg-NOAA
force-pushed
the
get_field_nc_indexing_bugfix
branch
from
7f9b66e to
e0fcbc7
Compare
This patch changes the intent of `values` from `intent(out)` to `intent(inout)` in `get_field_nc` and `netCDF_read_data_2d`. `read_netcdf_field` is left as `intent(out)`. This is due to the ambiguous shape of `values` in these two functions, which may represent either the compute or data domain. Because only the compute domain of `values` is filled by `get_field_nc`, it may contain residual values in its halos. Previous use of `intent(out)` would not necessarily preserve these values. This change explicitly directs the compiler to preserve them.
marshallward
force-pushed
the
get_field_nc_indexing_bugfix
branch
from
f92155e to
b111dc6
Compare
Hallberg-NOAA
pushed a commit
that referenced
this pull request
Oct 27, 2023
Remove fms_io_mod import
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This patch changes the intent of
valuesinget_field_ncfromintent(out)tointent(inout).Because only the compute domain of
valuesis filled byget_field_nc, it may contain residual values in its halos. Previous use ofintent(out)would not necessarily preserve these values. This change explicitly directs the compiler to preserve them.