search

JCSDA-internal / ioda-converters

Various converters for getting obs data in and out of IODA
8 stars 2 forks source link

merge smap9km modifications into original converter #1514

Closed BenjaminRuston closed 1 month ago

BenjaminRuston commented 2 months ago

build-group=https://github.com/JCSDA-internal/ioda/pull/1270

Description

merge the changes from the 9km smap surface soil moisture converter into the original

Issue(s) addressed

Resolves #1513

Impact

single converter for two flavors of SMAP surface soil moisture files

Dependencies

Checklist

YoulongXia-NOAA commented 2 months ago

@BenjaminRuston, Thank you for your effort. In particular for using ncks to reduce h5 file size into a small size file. I checked NRT smap soil moisture file and the format and variable names for both 9km SMAP and NRT SMAP files are the same. It makes sense to have a converter. As soil moisture converter is tested and used in land DA workflow only (by Darper's previous staff), it still need to tested within land DA workflow in the future.

BenjaminRuston commented 1 month ago

you nailed it @ClaraDraper-NOAA the EASE parameters are in the old file as well: DIFFER : VARIABLE : easeColumnIndex : DOES NOT EXIST IN "../../test/testoutput/smap_ssm.nc" DIFFER : VARIABLE : easeRowIndex : DOES NOT EXIST IN "../../test/testoutput/smap_ssm.nc" DIFFER : VARIABLE : surfaceQualifier : DOES NOT EXIST IN "../../test/testoutput/smap_ssm.nc" DIFFER : VARIABLE : vegetationOpacity : DOES NOT EXIST IN "../../test/testoutput/smap_ssm.nc"

I will update the testoutput file and this should not break anything for downstream users as they are requesting things by name as they need them extra bits are just ignored

YoulongXia-NOAA commented 1 month ago

@BenjaminRuston, that is great. As these two ioda converters are old not updated foe a long time. This is a great opportunity to get it updated. Thank you.

BenjaminRuston commented 1 month ago

I'm about done reconciling the differences... Here is what I've found and it relates entirely to the smap9km_ssm2ioda.py

the new output file from the decoder looks correct and has these features. The right side entries.

DIFFER : VARIABLE : vegetationOpacity : TYPE : INT <> FLOAT
DIFFER : TYPES : ATTRIBUTE : _FillValue : VARIABLE : vegetationOpacity : INT <> FLOAT

the vegetationOpacity should be a FLOAT this is correct correspondingly the values should be updated. Again the new files is the right entry:

vegetationOpacity      /MetaData
DIFFER : VARIABLE : vegetationOpacity : POSITION : [0] : VALUES : 0 <> 0.0423718 : PERCENT : 100
DIFFER : VARIABLE : vegetationOpacity : POSITION : [1] : VALUES : 0 <> 0.277997 : PERCENT : 100
DIFFER : VARIABLE : vegetationOpacity : POSITION : [26335] : VALUES : 0 <> 0.201079 : PERCENT : 100

so these will correct

BenjaminRuston commented 1 month ago

@ClaraDraper-NOAA where I have a question is regarding these values what should be used here.

smap_ssm2ioda.py ==> NRT file (testinput/SMAP_L2_SM_P_NRT_24342_A_20190822T224858_N16023_002.h5) smap9km_ssm2ioda.py ==> other file (testinput/SMAP_L2_SM_P_E_36365_D_20211122T013945_R18240_001.h5)

The NRT file uses 0.04 * soil_moisture The other file from smap9km_ssm9ioda.py had a constant of 0.04.

This update makes them consistent and they are now both 0.04 * soil_moisture is this correct?

soilMoistureVolumetric /ObsError
DIFFER : VARIABLE : soilMoistureVolumetric : POSITION : [0] : VALUES : 0.04 <> 0.00646782 : PERCENT : 83.8304
...
DIFFER : VARIABLE : soilMoistureVolumetric : POSITION : [26335] : VALUES : 0.04 <> 0.0016456 : PERCENT : 95.886
BenjaminRuston commented 1 month ago

then the error flags are altered between the two data types. The NRT file does not modify the PreQC values which come from the ncd.groups['Soil_Moisture_Retrieval_Data'].variables['retrieval_qual_flag']

while the smap9km_ssm2ioda.py in the same loop they modify the ObsError/soilMoistureVolumetric also modifies the quality flags setting those greater than 5 to 0:

        for i in range(len(lons)):
            if vals[i] > 0.0:
                # assumed 4% SM rather than -999.0
                errs[i] = 0.04*vals[i]
                if qflg[i] > 5:
                    qflg[i] = 0
                else:
                    qflg[i] = 1
            else:
                qflg[i] = 1

is this alteration of the quality flag also acceptable? is the PreQC even used?

BenjaminRuston commented 1 month ago

@srherbener and @gthompsnJCSDA this converter produced a file with units for latitude of degrees_north while the old file was degree_north

the current ObsSpace.yaml convention has degree_north

first off where is this converter getting this from???? It's not being explicitly set for it is coming from the writer?

gthompsnJCSDA commented 1 month ago

@BenjaminRuston Singular and plural versions are interchangeable. If the units are not explicitly set in the converter, then I am guessing the ioda (engines perhaps) is doing the assignment of those units by default. I believe it is better to ensure the converter sets units for all appropriate variables rather than letting another program do it mysteriously.

BenjaminRuston commented 1 month ago

@gthompsnJCSDA ... duh!!!

just wrote the units in the converter:

        self.varAttrs[('latitude', 'MetaData')]['units'] = 'degree_north'
        self.varAttrs[('longitude', 'MetaData')]['units'] = 'degree_east'

so that fixed that and now match convention... thanks!

@srherbener why does the writer.BuildIoda set degrees_north do you know where to change that? We should have it match convention

BenjaminRuston commented 1 month ago

From @ClaraDraper-NOAA :

The error should be constant at 0.04 m3/m3 (or 4%), and not 4% of the obs.

awesome thanks

YoulongXia-NOAA commented 1 month ago

then the error flags are altered between the two data types. The NRT file does not modify the PreQC values which come from the ncd.groups['Soil_Moisture_Retrieval_Data'].variables['retrieval_qual_flag']

while the smap9km_ssm2ioda.py in the same loop they modify the ObsError/soilMoistureVolumetric also modifies the quality flags setting those greater than 5 to 0:

        for i in range(len(lons)):
            if vals[i] > 0.0:
                # assumed 4% SM rather than -999.0
                errs[i] = 0.04*vals[i]
                if qflg[i] > 5:
                    qflg[i] = 0
                else:
                    qflg[i] = 1
            else:
                qflg[i] = 1

is this alteration of the quality flag also acceptable? is the PreQC even used?

@BenjaminRuston, use smap9km_ssm2ioda.py as a right converter without change PreQC as did in smap9km_ssm2ioda.py as it is correct one. The smap_ssm2ioda.py is a very old version. Thank you.

BenjaminRuston commented 1 month ago

thank you @YoulongXia-NOAA for the help sorting this out. This should be all set to go now

BenjaminRuston commented 1 month ago

@YoulongXia-NOAA please have a look this is ready

YoulongXia-NOAA commented 1 month ago

@YoulongXia-NOAA please have a look this is ready

@BenjaminRuston, please remove the sentence

self.varAttrs[('surfaceQualifier', 'MetaData')]['units'] = 'unitless'

and check if vals is used in get_observation_time(self.filename, ncd, vals)

The others are good. If you fix them, I will approve this PR. Thank you.

BenjaminRuston commented 1 month ago

@YoulongXia-NOAA yes vals is used in the get_observation_time function for the case when the tb_time_seconds is not in the variable keys

BenjaminRuston commented 1 month ago

regarding the removal of the attribute for surfaceQualifier if this line is removed: https://github.com/JCSDA-internal/ioda-converters/blob/7341b0c27de96127b2391b2f386f1104cd300961/src/land/smap_ssm2ioda.py#L143

the error is then generated:

DIFFER : VARIABLE "surfaceQualifier" IS MISSING ATTRIBUTE WITH NAME "units" IN FILE

@gthompsnJCSDA what is the unit to use for this? It is a unitless integer ?

YoulongXia-NOAA commented 1 month ago

@BenjaminRuston @gthompsnJCSDA, I checked ObsSpace.yaml, its definition is below:

BenjaminRuston commented 1 month ago

@BenjaminRuston @gthompsnJCSDA, I checked ObsSpace.yaml, its definition is below:

  • Variable: [ "surfaceQualifier", "surface_type" ] # 0=land, 1=sea, 2=seaice MetaData: true Type: Enum It is the integer but without unit. It should be a variable without unit attribute, right?

@YoulongXia-NOAA think we may want to change the ObsSpace.yaml

@gthompsnJCSDA for this surfaceQualifier do we want to change its ObsSpace.yaml entry to this:

    Force Units: false
    Type: UInt32
    MetaData: true
YoulongXia-NOAA commented 1 month ago

@BenjaminRuston @gthompsnJCSDA, I checked ObsSpace.yaml, its definition is below:

  • Variable: [ "surfaceQualifier", "surface_type" ] # 0=land, 1=sea, 2=seaice MetaData: true Type: Enum It is the integer but without unit. It should be a variable without unit attribute, right?

@YoulongXia-NOAA think we may want to change the ObsSpace.yaml

@gthompsnJCSDA for this surfaceQualifier do we want to change its ObsSpace.yaml entry to this:

    Force Units: false
    Type: UInt32
    MetaData: true

@BenjaminRuston, we should not change ObsSpace.yaml just removed that units sentence. However, you told me that when you remove that sentence, this brings the error. I assumed that remove this sentence should not bring error. we need to solve why it produce the error, right?

YoulongXia-NOAA commented 1 month ago

@BenjaminRuston, I may be wrong. If the change you suggested for obsSpace.yaml can solve this issue, it is great. Thank you.

YoulongXia-NOAA commented 1 month ago

@BenjaminRuston, @gthompsnJCSDA, I checked ObsSpace.yaml file, for flag and type alike variable, they use MetaData: true Type: Enum

But for Index alike variables, they use Type: UInt32 MetaData: true

I am not sure what differences between the two. However, both of them has no units definition as they has no unit.

BenjaminRuston commented 1 month ago

thanks @YoulongXia-NOAA ..... now .... we've removed unitless and updated the ObsSpace.yaml please also review the IODA PR: https://github.com/JCSDA-internal/ioda/pull/1270

and this is ready ;0)

BenjaminRuston commented 1 month ago

btw, talked to @gthompsnJCSDA and he advised to leave this as 

Type: Enum

and just add the:

 Force Units: false

so that is what I did

YoulongXia-NOAA commented 1 month ago

btw, talked to @gthompsnJCSDA and he advised to leave this as

Type: Enum

and just add the:

 Force Units: false

so that is what I did

Okay, I approved this PR now.

BenjaminRuston commented 1 month ago

@haydenlj or @gthompsnJCSDA could you have a look over this PR

YoulongXia-NOAA commented 1 month ago

Thank you, @BenjaminRuston. You made it done.