MetOffice / opsinputs

JEDI library generating VarObs and Cx files
BSD 3-Clause "New" or "Revised" License
4 stars 0 forks source link

Fix target variable for BriTempVarError #129

Closed smnewman closed 1 year ago

smnewman commented 1 year ago

This PR fixes a bug encountered when running the VarObs Writer filter with ops-um-jedi executable: this results in missing values for BriTempVarError.

There is a problem extracting observation errors from ObsError/brightness_temperature. This appears not to work with obsspace_get_db routines.

Instead, many filters in ufo are written such that the ObsErrorData group is accessed. That is the approach followed here, i.e. replacing the target variable ObsError/brightness_temperature with ObsErrorData/brightness_temperature.

smnewman commented 1 year ago

To clarify how this change has been tested: A sample yaml for ATOVS has been run with unifiedmodel_hofx.x. Without the fix, the contents of BriTempVarError in the output VarObs file contain only missing data. With the fix, the variable is written correctly. The modified yaml looks like this:

    - filter: Variable Assignment
      assignments:
      - name: ObsErrorData/brightness_temperature
        channels: *all_channels
        type: float
        function:
          name: ObsFunction/MetOfficeAllSkyErrorModel
          options:
            channels: *all_channels
            fixland:    [0.0000,  0.0000,  0.0000,  0.8000,  0.3000,  0.2500,  0.2000,  0.0000,  0.0000,  0.0000,
                         0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.5400]
            fixsea:     [0.0000,  0.0000,  0.0000,  0.8000,  0.2000,  0.2000,  0.2000,  0.0000,  0.0000,  0.0000,
                         0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000]
            fixice:     [0.0000,  0.0000,  0.0000,  0.8000,  0.3000,  0.2500,  0.2000,  0.0000,  0.0000,  0.0000,
                         0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000]
            taulinsea:  [0.0000,  0.0000,  0.0000,  0.6000,  0.4000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,
                         0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000]
            taulinland: [0.0000,  0.0000,  0.0000,  2.3000,  2.3000,  2.3000,  0.0000,  0.0000,  0.0000,  0.0000,
                         0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  2.2000,  2.2000,  2.3000]
            taulinice:  [0.0000,  0.0000,  0.0000,  3.2000,  3.2000,  3.2000,  0.0000,  0.0000,  0.0000,  0.0000,
                         0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000]
            tausqsea:   [0.0000,  0.0000,  0.0000,  3.2000,  3.2000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,
                         0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000]
            tausqland:  [0.0000,  0.0000,  0.0000,  4.2000,  5.4000, 12.0000,  0.0000,  0.0000,  0.0000,  0.0000,
                         0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  6.4000,  6.4000,  6.3000]
            tausqice:   [0.0000,  0.0000,  0.0000, 10.5000, 11.9000, 15.4000,  0.0000,  0.0000,  0.0000,  0.0000,
                         0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000]
            lwpcoef:    [0.0000,  0.0000,  0.0000,  0.3225,  0.1040,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,
                         0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  0.0000,  1.0959,  0.7149,  0.3567]
            iwpcoef:    [0.0000,  0.0000,  0.0000,  0.2563,  0.0996,  0.1377,  0.0184,  0.0383,  0.0724,  0.0727,
                         0.2610,  0.4064,  0.7837,  1.7191,  0.0000,  0.0000,  0.0000,  0.3466,  0.1963,  2.2967]
            ScaleVarErrorAboveEmissError: 0.025
            UseEmissivityAtlas: true
            maxiwp: 2.0
smnewman commented 1 year ago

change looks good, but there is one failure in the testing. Its unlikely that is this update- what do you think?

It looks like the CI failure is related to https://github.com/JCSDA-internal/jedi-cmake/pull/27. Do you know @mikecooke77 if we need to fix this ourselves?

mikecooke77 commented 1 year ago

change looks good, but there is one failure in the testing. Its unlikely that is this update- what do you think?

It looks like the CI failure is related to JCSDA-internal/jedi-cmake#27. Do you know @mikecooke77 if we need to fix this ourselves?

change looks good, but there is one failure in the testing. Its unlikely that is this update- what do you think?

It looks like the CI failure is related to JCSDA-internal/jedi-cmake#27. Do you know @mikecooke77 if we need to fix this ourselves?

I think @matthewrmshin has a fix https://github.com/MetOffice/opsinputs/pull/130. So I think we merge @matthewrmshin PR and then update to make sure all is OK with this one

DJDavies2 commented 1 year ago

I'm getting a ctest failure:

test_opsinputs_varobswriter_globalnamelist_atovs:

Completed case 0: ufo/ObsFilters/testFilters FAILED: ufo/ObsFilters/testFilters 1 tests failed out of 1. Finished running the unit tests, result = 1 ATOVS : no output

smnewman commented 1 year ago

@brettcandy @stemiglio I updated the yaml for ctest test_opsinputs_varobswriter_globalnamelist_atovs and CI checks now pass.

mikecooke77 commented 1 year ago

@matthewrmshin and @yaswant any ideas why the build is failing. It looks like something to so with shumlib?

matthewrmshin commented 1 year ago

The error says ecbuild: command not found at the command to configure the mini bundle. (This is after it has finished building shumlib which does not use ecbuild). So most likely due to changes to where ecbuild is located in PATH.

yaswant commented 1 year ago

We are using jcsda container image, and it looks like ecbuild is missing from the recent image (Sept 30 update).

yaswant commented 1 year ago

@matthewrmshin for now if you update the Dockerfile to use image with revert tag that should work, I think.

diff --git a/Dockerfile b/Dockerfile
index 3391f2d..6f5e545 100644
--- a/Dockerfile
+++ b/Dockerfile
@@ -1,4 +1,4 @@
-FROM jcsda/docker-gnu-openmpi-dev:latest
+FROM jcsda/docker-gnu-openmpi-dev:revert
matthewrmshin commented 1 year ago

See my change in #131 for a temporary fix.

yaswant commented 1 year ago

@matthewrmshin scratch that suggestion. I consulted with Dom and it appears that with the new container, we need to source /etc/profile.d/z10_spack_environment.sh before running the build task.

yaswant commented 1 year ago

135 should fix the ci (build/test) issue when develop is merged in.