Adding soil depth to two smap ioda converters by YoulongXia-NOAA · Pull Request #1075 · JCSDA-internal/ioda-converters

YoulongXia-NOAA · 2022-10-07T19:54:48Z

Description

Based on the land DA users' request, the soil depth named as depthBelowSurface was added into smap9km_ssm2ioda.py and smap_ssm2ioda.py. Note that this addition is created from the develop branch with out new conventions and standard modification.

Issue(s) addressed

Link the issues to be closed with this PR

The issue is Add observation depth to SMAP soil moisture IODA converter #1068

Acceptance Criteria (Definition of Done)

ctest passed and updated covers are merged into the develop branch

Dependencies

No

Impact

No

YoulongXia-NOAA · 2022-10-07T20:30:31Z

All,
the two smap converters are updated via adding soil depth (0.025 m). When your time is available, could you please review this PR? Thank you and have a great weekend.

ClaraDraper-NOAA

Looks good to me, thanks @YoulongXia-NOAA

zcstanley

Thanks @YoulongXia-NOAA, this looks good to me as well!

YoulongXia-NOAA · 2022-10-12T14:40:38Z

Thank you, @ClaraDraper-NOAA and @zcstanley for your look. After Stephen takes a look and approves, I will be merged into the develop. In that case, you can test it for your practical application.

YoulongXia-NOAA · 2022-10-17T15:30:35Z

@srherbener, do you have any chance to look at this PR? I know that you are very busy and therefore here just friendly asking and reminding if your time is available, please take a look. Thank you.

srherbener

Thanks for updates. I have a couple suggestions that should eliminate unnecessary operations.

src/land/smap9km_ssm2ioda.py

src/land/smap_ssm2ioda.py

srherbener

Thanks for addressing my comments!

BenjaminRuston · 2022-10-18T14:52:17Z

src/land/smap9km_ssm2ioda.py

        ecoli = ncd.groups['Soil_Moisture_Retrieval_Data'].variables['EASE_column_index'][:].ravel()
        refsec = ncd.groups['Soil_Moisture_Retrieval_Data'].variables['tb_time_seconds'][:].ravel()

+        deps = np.full_like(vals, 0.025)


what is the repeating value of 0.025 a default value? could this be set somewhere with a longer name?

@BenjaminRuston, this is what Clara Drape @ClaraDraper-NOAA r asked me to do (see issue #1068.

BenjaminRuston · 2022-10-18T14:52:54Z

src/land/smap_ssm2ioda.py

        errs = ncd.groups['Soil_Moisture_Retrieval_Data'].variables['soil_moisture_error'][:].ravel()
        qflg = ncd.groups['Soil_Moisture_Retrieval_Data'].variables['retrieval_qual_flag'][:].ravel()
+
+        deps = np.full_like(vals, 0.025)


here's that same value,,, may be good to centralize its value to a constant somewhere

Not sure what this means. @ClaraDraper-NOAA, could you please give a response for this comment.

Can we set a variable called:

assumed_obsevation_depth = 0.025 #observation assumed to be 2.5cm or 0.025m

and then use this in the two places where it appears... makes it easier to possible make this be something which can be passed in via yaml or arguments

@BenjaminRuston, based on Greg Thomson's convention table, it is defined as depthBelowSoilSurface. Here is only abbreviation used in python code. In ioda netcdf4 file, its name is depthBelowSoilSurface.

If you want me to add comments like this: deps = np.full_like(vals, 0.025) #observation assumed to be 0.025m, I can add it to my python code. Do you really want @BenjaminRuston?

but the number 0.025 appears in two separate places so it seems like a good one to consolidate so they point to a single variable, don't you think?

sure using depthBelowSoilSurface is fine, but would you agree this may be better than having 0.025 hard coded in two separate locations.... and yes long names because someone new may not immediately know what deps means

Okay, you mean that I can define it as assumed_obsevation_depth = 0.025 #observation assumed to be 2.5cm or 0.025m in the beginning of this python code, and then put the constant value via deps = np.full_like(vals, assumed_obsevation_depth), right? @BenjaminRuston

@srherbener, in two separate locations.... , I do not understand this sentence. As there are two converters, one is for NRT SMAP data, and the other one is for 9km SMAP data. For each converter, it only uses once. I am not sure what do you want to consolidate? @ClaraDraper-NOAA, @zcstanley , or @srherbener, who can explain a bit more? I am confusing by this comment (I am sorry).

@YoulongXia-NOAA no worries. I think what @BenjaminRuston means by "in two separate locations..." is that the hard coded value 0.025 appears once in each of the two scripts (smap9km_ssm2ioda.py and smap_ssm2ioda.py). I think what is being suggested is to have a parameter that is defined in only one place and used by both of the scripts.

We should have a common (shared) file for situations like this where you define common parameters to be used in multiple scripts. I'm not sure if we have that yet for the converters. This would be a nice long term solution.

Perhaps for the short term it would be acceptable to define the parameter at the beginning of each script so that if it ever gets used in multiple places within a script you've got an easier way to update the value. Thinking about a shared file perhaps a good name for the parameter would be something like assumedSoilDepth = 0.025. Then when the shared file scheme gets implemented, you would simply delete the setting of the parameter at the beginning to the two scripts and import the shared file with the parameter setting.

Setting up the shared parameter file could be done in a subsequent PR after this one.

Does this sound like an acceptable approach?

Thank you, @srherbener. I do need to learn this part in the future. As I did not have such an experience, so that I could not catch the Ben's comments. Additionally, for the future update, I may also input it as parameter from start like this:

parser.add_argument('-D', '--depth',
help="assumedSoilDeppth", type=str, required=True)
in the code, convert string into float. This will leave it as input argument. Such a update will need the guidance and instructions. Thank you @BenjaminRuston for your comments and give a new view.

BenjaminRuston · 2022-10-18T14:54:04Z

@YoulongXia-NOAA is there an associated ufo-data PR with the data for the ctest

YoulongXia-NOAA · 2022-10-18T15:44:28Z

@YoulongXia-NOAA is there an associated ufo-data PR with the data for the ctest

@BenjaminRuston, I only worked on ioda-comverter but not ufo-data. @ClaraDraper-NOAA, who will do this test?

ClaraDraper-NOAA · 2022-10-18T17:42:21Z

@YoulongXia-NOAA is there an associated ufo-data PR with the data for the ctest

@BenjaminRuston, I only worked on ioda-comverter but not ufo-data. @ClaraDraper-NOAA, who will do this test?

@YoulongXia-NOAA I wasn't aware that there was a second repo, but am assuming that ufo-data is where the output of the converters go (right, @BenjaminRuston ) , so I think you probably just need to create a second PR for that repo with. the updated output from the converter.

YoulongXia-NOAA · 2022-10-18T22:58:30Z

@BenjaminRuston, I make changes here much faster than you create the new branch. I just need how to do such a change. Please review my changes and see if this is what you want me to do. Thank you.

BenjaminRuston · 2022-10-18T23:37:27Z

test/CMakeLists.txt

                            -i testinput/SMAP_L2_SM_P_E_36365_D_20211122T013945_R18240_001.h5
                            -o testrun/smap9km_ssm.nc
-                            -m maskout"
+                            -m maskout


really think passing the string maskout is a bit odd and yes can change here but would recommend adding just the -m argument which changes a boolean to true and removing the depth entirely and have the default be in the python converter

it is potentially a bit hidden but do not want people thinking they can change the retrieval somehow

@BenjaminRuston I'm also OK with hard coding the SMAP depth, since this is a fixed characteristic of L-band. The only reason I can think of that the observed depth would change would be if we changed the interpolation algorithms to no longer act on the midpoint of each layer - but this would be a much larger code change anyway. We might also want to trick the interpolation into using only model layer 1 in h(x), even if layer 1 is not 5 cm, by setting the observed depth to whatever the depth of layer 1 is, but this is an ugly work-around, so I think being forced to change the default value in this case is appropriate.

@BenjaminRuston, I would rather than keep this flexibility for the user to avoid often modify code. We have a default option, if you select -m default. It is more flexible for the depth is not exact the same 0.025m, as Clara said for interpolation or something else. We can update README.md to tell the user how to use it. This should totally fine, I think.

This is my opinion. If we want to update, certainly user friendly update is preferred.

I do not think CMake is user friendly and a ctest is not what people should be modifying unless they are changing the file.

the converter should have the flexibility... I guess I will talk with Clara about this and we can make the call

please change and remove the string argument maskout and make it a boolean

if you like I can push changes up directly to your branch

YoulongXia-NOAA · 2022-10-19T00:34:46Z

Ben, please go ahead to fit push your change. We can discuss other part. Thank you

On Tue, Oct 18, 2022 at 8:16 PM Benjamin Ruston ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test/CMakeLists.txt <#1075 (comment)> : > @@ -1745,7 +1746,8 @@ ecbuild_add_test( TARGET test_${PROJECT_NAME}_smap9km_ssm "${Python3_EXECUTABLE} ${CMAKE_BINARY_DIR}/bin/smap9km_ssm2ioda.py -i testinput/SMAP_L2_SM_P_E_36365_D_20211122T013945_R18240_001.h5 -o testrun/smap9km_ssm.nc - -m maskout" + -m maskout if you like I can push changes up directly to your branch — Reply to this email directly, view it on GitHub <#1075 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AMKGNUG6ZTAEH2E6OASCOELWD44XHANCNFSM6AAAAAAQ737MRE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- ================================================================ Youlong Xia, Ph.D SAIC at EMC/NCEP 5830 University Research Court College Park, MD 20740 Phone: 301-683-3696 =================================================================

YoulongXia-NOAA · 2022-10-19T19:05:37Z

@BenjaminRuston, did you run ioda-bundle for thes etwo converters? They have errors below:
Traceback (most recent call last):
File "/home/nonroot/bin/smap_ssm2ioda.py", line 184, in
main()
File "/home/nonroot/bin/smap_ssm2ioda.py", line 162, in main
optional.add_argument(
File "/opt/software/linux-ubuntu20.04-skylake_avx512/gcc-9.4.0/python-3.9.12-4xzpp3b2v73sg7up3kxjwwee4a6r6deb/lib/python3.9/argparse.py", line 1422, in add_argument
action = action_class(**kwargs)
TypeError: init() got an unexpected keyword argument 'type'

YoulongXia-NOAA · 2022-10-19T19:21:59Z

@BenjaminRuston, I run ioda-bundle on Hera, get the same errors:

File "/scratch2/NCEPDEV/stmp3/Youlong.Xia/CoverterSMAP/ioda-bundle/build/bin/smap_ssm2ioda.py", line 165, in main
type=bool, default=False, action='store_true', required=False)
File "/scratch1/NCEPDEV/da/python/opt/core/miniconda3/4.6.14/envs/gdasapp/lib/python3.7/argparse.py", line 1359, in add_argument
action = action_class(**kwargs)
TypeError: init() got an unexpected keyword argument 'type'

Could you please check which type is not correct? (I suspect type=bool, but I am not sure)

BenjaminRuston · 2022-10-20T23:30:04Z

@YoulongXia-NOAA yes you are correct the type=bool is unnecessary,, feel free to make the change in the future.

Please let me know if this is working for you, and feel free to make changes if you see a problem.

YoulongXia-NOAA · 2022-10-21T12:49:30Z

@BenjaminRuston, thank you for you fixing this error. Now these converters pass ctests. I alsp update my local repository, and run them on Hera without error. I made corresponding changes for README.md to inform the users how to use these two converters.

YoulongXia-NOAA · 2022-10-21T16:37:10Z

@BenjaminRuston, could you please approve this PR if you think that it is ready? If so, Steven @srherbener can merge it into develop so that Zofia @zcstanley can test for her application when his time is available.

BenjaminRuston · 2022-10-24T14:26:52Z

@YoulongXia-NOAA merging,, but note we should look to combine these converters as they only differ by about 12 lines!

YoulongXia-NOAA · 2022-10-24T14:40:30Z

@BenjaminRuston, thank you for your merging. In general my land-related converters were following the previous examples I learnt from marine component when I started to work on ioda-converters in 2020. This time I got some new experience for ioda-converter. In future, I may need to modify them in the similar way here for the others when I need to update them.

BenjaminRuston · 2023-01-23T18:26:45Z

this is a soil moisture product but is a start toward the 9km Tb converter

Adding soil depth to two smap ioda converters

2245476

YoulongXia-NOAA assigned YoulongXia-NOAA, ClaraDraper-NOAA, zcstanley and srherbener Oct 7, 2022

YoulongXia-NOAA added AOP22 LAND Land DA OBS OBS processing, UFO labels Oct 7, 2022

YoulongXia-NOAA added this to the LAND - October 2021 milestone Oct 7, 2022

YoulongXia-NOAA linked an issue Oct 7, 2022 that may be closed by this pull request

Add observation depth to SMAP soil moisture IODA converter #1068

Closed

YoulongXia-NOAA requested review from ClaraDraper-NOAA, srherbener and zcstanley October 7, 2022 19:58

Update testout nc files after updated convertes

3cefd0c

ClaraDraper-NOAA approved these changes Oct 7, 2022

View reviewed changes

zcstanley approved these changes Oct 12, 2022

View reviewed changes

srherbener reviewed Oct 17, 2022

View reviewed changes

src/land/smap9km_ssm2ioda.py Outdated Show resolved Hide resolved

src/land/smap_ssm2ioda.py Outdated Show resolved Hide resolved

Update smap conterter with the reviewer's comments

cf21b76

srherbener approved these changes Oct 17, 2022

View reviewed changes

BenjaminRuston reviewed Oct 18, 2022

View reviewed changes

Update SMAP ioda converters based on the reviewer's comments

7f44cfb

BenjaminRuston reviewed Oct 18, 2022

View reviewed changes

BenjaminRuston added 2 commits October 19, 2022 00:55

make mask argument boolean remove depth option from ctest

d296776

make optional argument optional

186f4da

remove unnecessary and problematic boolean type

c78b261

Made the corresponding changes in README.md for SMAP converters

3382d23

BenjaminRuston merged commit 8e8d7bd into develop Oct 24, 2022

BenjaminRuston deleted the feature/smap_soilDepth branch October 24, 2022 14:26

Conversation

YoulongXia-NOAA commented Oct 7, 2022

Description

Issue(s) addressed

Acceptance Criteria (Definition of Done)

Dependencies

Impact

Uh oh!

YoulongXia-NOAA commented Oct 7, 2022

Uh oh!

ClaraDraper-NOAA left a comment

Choose a reason for hiding this comment

Uh oh!

zcstanley left a comment

Choose a reason for hiding this comment

Uh oh!

YoulongXia-NOAA commented Oct 12, 2022

Uh oh!

YoulongXia-NOAA commented Oct 17, 2022

Uh oh!

srherbener left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

srherbener left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenjaminRuston commented Oct 18, 2022

Uh oh!

YoulongXia-NOAA commented Oct 18, 2022

Uh oh!

ClaraDraper-NOAA commented Oct 18, 2022

Uh oh!

YoulongXia-NOAA commented Oct 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YoulongXia-NOAA commented Oct 19, 2022 via email

Uh oh!

YoulongXia-NOAA commented Oct 19, 2022

Uh oh!

YoulongXia-NOAA commented Oct 19, 2022

Uh oh!