Skip to content

enhancement/bugfix: use NetCDF missing values instead of -9999#570

Merged
grantfirl merged 8 commits intoNCAR:mainfrom
scrasmussen:enhancement/netcdf_missing_value_precisions
Apr 29, 2025
Merged

enhancement/bugfix: use NetCDF missing values instead of -9999#570
grantfirl merged 8 commits intoNCAR:mainfrom
scrasmussen:enhancement/netcdf_missing_value_precisions

Conversation

@scrasmussen
Copy link
Copy Markdown
Member

DESCRIPTION OF CHANGES:

  • Using NetCDF variables for missing_values instead of -9999.0 and -9999
  • Fixing/adding generic interfaces for single and double precision. Because the generic interface allow a single subroutine name to be used to call using variables of single or double precision, missing_value_sp and missing_value_dp variables need to be used in the subroutine definitions.
  • Added generic interface definitions for NetCDF_read_att_dp, NetCDF_put_var_1d_sp, NetCDF_put_var_2d_sp, NetCDF_put_var_1d_dp, NetCDF_put_var_2d_dp, check_missing_real_sp_0d, check_missing_real_sp_1d, conditionally_set_real_sp_var_0d, conditionally_set_real_sp_var_1d

ISSUES: Related to issues brought up in Issue #519

TESTS: compiles both single and double precision build setup

@scrasmussen scrasmussen force-pushed the enhancement/netcdf_missing_value_precisions branch from 12363db to 63e7653 Compare March 20, 2025 22:15
Copy link
Copy Markdown
Member

@dustinswales dustinswales left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.
The precision police abide :)

@hertneky
Copy link
Copy Markdown
Collaborator

@scrasmussen @grantfirl I ran tests using some of Wayne's files. His files set a fill value that's quite large for the surface forcing variables e.g.:

        float hfss(time) ;
                hfss:_FillValue = 9.96921e+36f ;

With this PR, that of course causes the missing_value/missing_value_eps to be large and therefore causes this error to trigger, even when there are realistic flux values.

    if (maxval(input_force_sfc_sens_flx(:)) < missing_value_eps) then
      write(*,*) 'The global attribute surfaceForcing in '//trim(adjustl(scm_state%case_name))//'.nc indicates that the variable sfc_sens_flx should be present, but it is missing. Stopping ...'
      error stop "The global attribute surfaceForcing in indicates that the variable sfc_sens_flx should be present, but it is missing."

This is just one example in the code of many where these types of checks are done. Is there another way to check if the value is missing. Is there issue with checking if var == missing_value, as I see in other places in the code? I understand that this method may not be reliable due to precision errors of floating point numbers, so probably not!

I will note that the noflux case did run through to completion and produced output, since it did not get caught up in any of these missing_value comparisons.

@scrasmussen scrasmussen force-pushed the enhancement/netcdf_missing_value_precisions branch from 5e35c49 to ff31f8e Compare April 7, 2025 18:03
@scrasmussen
Copy link
Copy Markdown
Member Author

Is there issue with checking if var == missing_value, as I see in other places in the code? I understand that this method may not be reliable due to precision errors of floating point numbers, so probably not!

@hertneky
If the comparison uses variables of a different data type there might be an issue but this can be worked around with generic interfaces.

This commit adds generic procedures to report true/false if a variable is a missing value. The generic interfaces mean that it can choose the same datatype for MISSING_VALUE as the input value. Using a is_missing_value procedure also helps with readability.

@scrasmussen scrasmussen force-pushed the enhancement/netcdf_missing_value_precisions branch from ff31f8e to 403e097 Compare April 7, 2025 18:43
@grantfirl
Copy link
Copy Markdown
Collaborator

@scrasmussen I think that scrasmussen#8 should fix whatever issue was showing up in the RTs. I ran them on my local machine and they ran fine.

…_precisions_2

Minor changes to Soren's PR
@grantfirl
Copy link
Copy Markdown
Collaborator

@scrasmussen CI RTs are still failing. Hmm. I can't reproduce the failure on my machine, which is odd. I'm re-running the debug mode RTs only to see if it's only the single precision tests failing (which kills the associated tests that take longer).

@grantfirl
Copy link
Copy Markdown
Collaborator

@scrasmussen I can now reproduce some of the errors in the CI tests after re-downloading the case data files. I'm looking into it.

@grantfirl
Copy link
Copy Markdown
Collaborator

@scrasmussen Still working on a fix for this. There are many places that had been assuming -999 that still exist in the code that I'm fixing. There will be another PR into your branch shortly.

@grantfirl
Copy link
Copy Markdown
Collaborator

@scrasmussen @hertneky Please see scrasmussen#9. It updates this PR branch to the latest main and has many fixes for surface variable initialization that no longer rely on checking for -999 or <0.

…_precisions

More fixes for missing value branch
@grantfirl
Copy link
Copy Markdown
Collaborator

@scrasmussen @hertneky Using nccmp -d on manually-run RT output shows that the differences are only in the missing data values, as hypothesized. I didn't check all files, but the ones that I did showed this.

@grantfirl
Copy link
Copy Markdown
Collaborator

@scrasmussen @hertneky Using nccmp -d on manually-run RT output shows that the differences are only in the missing data values, as hypothesized. I didn't check all files, but the ones that I did showed this.

Some tests don't show any differences with nccmp -d -f (the data is identical) but do show differences with nccmp -m -f (metadata is different - specifically missing value attributes). This is as-expected, and this can be merged.

@grantfirl grantfirl merged commit d4d6cc3 into NCAR:main Apr 29, 2025
16 checks passed
@grantfirl
Copy link
Copy Markdown
Collaborator

@hertneky @scrasmussen @mkavulich Please update the baselines on mohawk with artifacts from https://github.com/NCAR/ccpp-scm/actions/runs/14599498366

@scrasmussen
Copy link
Copy Markdown
Member Author

@hertneky @scrasmussen @mkavulich Please update the baselines on mohawk with artifacts from https://github.com/NCAR/ccpp-scm/actions/runs/14599498366

Updated the baselines, thanks for your work on this @grantfirl!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants