Refactor of get_field_nc index fixes #8
Conversation
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 #8 +/- ##
=============================================================
Coverage 37.16% 37.16%
=============================================================
Files 265 265
Lines 74511 74516 +5
Branches 13839 13839
=============================================================
+ Hits 27689 27691 +2
- Misses 41728 41731 +3
Partials 5094 5094
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
marshallward
force-pushed
the
get_field_nc_indexing_bugfix
branch
from
aef5958 to
96bc99d
Compare
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
marshallward
force-pushed
the
get_field_nc_indexing_bugfix
branch
from
96bc99d to
d59b751
Compare
| iscl = 1 ; iecl = iec+1-isc ; jscl = 1 ; jecl = jec+1-jsc | ||
| endif | ||
| if (data_domain) & | ||
| allocate(values_c(1:iec-isc+1,1:jec-jsc+1)) |
There was a problem hiding this comment.
Because values is intent(out), won't removing the values(:,:) = 0.0 statement that was here before any of these changes lead to uninitialized values (and perhaps NaNs) appearing it its halo regions when data_domain is true, even if the incoming argument is properly initialized outside of this routine? (In practice this might be fine with some compilers if they treat intent(out) like intent(inout).) If so, even a halo update may not be able to eliminate these uninitialized variables.
Were values intent(inout), I would agree that removing its initialization statement should be benign, if possibly outside of the scope of what is strictly necessary in this bug-fix PR.
There was a problem hiding this comment.
Thank you for this detailed feedback.
Renaming the previous internal variable name values_nc to values_c (which is most of the lines that were changed) seems like it is stretching the scope of this PR a bit, but otherwise it seems fine.
If the initialization of values is to be removed when it is passed in on the data domain, I believe that its intent needs to be changed from intent(out) to intent(inout) so that its halo regions are not inadvertently filled with uninitialized values, regardless of what had been done to initialize an array before it is passed to this routine. As this would be a larger conversation extending beyond the scope of this limited bug-fix PR, I am going to accept these changes but immediately amend them to restore this initialization of values to its status in dev/gfdl from before this PR. If we wish to persist in removing this initialization, I think that it needs to be done in a separate PR that can be evaluated separately on its own merits, and with a clear traceback for future git bisection of any bugs that might arise as a result.
|
I agree |
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
This patch addresses some of the proposed changes to
get_field_nc.WARNINGis removed fromuse MOM_error_handlersince it is unused.Zero initialization of
valueshas been removedI find no evidence that
MOM_read_dataor its principal function call,mpp_read, apply any initialization to the halo data. Also, there is no reason why the user should expect aread()-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_cbysource=has been removed.This initialization serves no purpose, since every value will be filled by the netCDF read statement.
values_cindexing is now 1-basedvalueshas ambiguous shape, so it can follow no natural indexing. Althoughvalues_calways resides on the compute domain, the transfer tovaluesmay 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_ctovalues.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_ncrenamed tovalues_cPrevious 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