Add the capability to assimilate the MADIS snow depth data from GTS #1836

jiaruidong2017 · 2023-09-05T20:37:22Z

This PR updates the land_analysis.py to add the capability to assimilate the madis snow depth data from GTS.

This PR depends on GDASApp #602

How has this been tested?

Clone and build on Hera
Cycled test on Hera Run C384/C192 L127 for 2023050100.
Observations on hera at /scratch1/NCEPDEV/global/Jiarui.Dong/JEDI/GlobalWorkflow/para_gfs/glopara_dump/gdas.20230501/

Checklist

Any dependent changes have been merged and published
My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
My changes generate no new warnings
New and existing tests pass with my changes
I have made corresponding changes to the documentation if necessary

WalterKolczynski-NOAA

Each of these different steps should probably be broken into individual methods that prepare_GTS calls, as it is getting a bit long now and doing multiple things. Will see what @aerorahul thinks.

WalterKolczynski-NOAA · 2023-09-08T19:17:30Z

ush/python/pygfs/task/land_analysis.py

+        if os.path.isfile(f"{os.path.join(localconf.DATA, output_file)}"):
+            rm_p(output_file)


Suggested change

if os.path.isfile(f"{os.path.join(localconf.DATA, output_file)}"):

rm_p(output_file)

rm_p(output_file)

The above lines were removed.

aerorahul

Mostly looks good. We can work to make it better and data driven.

aerorahul · 2023-09-08T18:58:34Z

parm/config/gfs/config.preplandobs

+export BUFRADPSFCYAML="${HOMEgfs}/sorc/gdas.cd/test/testinput/bufr_adpsfc_snow.yaml"
+export BUFRSNOCVRYAML="${HOMEgfs}/sorc/gdas.cd/test/testinput/bufr_snocvr.yaml"


Should these yamls be in parm/land/ and not test/?

All the GTS bufr2ioda yamls in GDASApp are saved at the test/testinput/ directory.

aerorahul · 2023-09-08T19:00:53Z

ush/python/pygfs/task/land_analysis.py

+        logger.info(f"Executing {exe}")
+        try:
+            exe()
+        except OSError:
+            raise OSError(f"Failed to execute {exe}")
+        except Exception:
+            raise WorkflowException(f"An error occured during execution of {exe}")
+
+        output_file = f"{localconf.OPREFIX}snocvr_snow.nc4"
+        if os.path.isfile(f"{os.path.join(localconf.DATA, output_file)}"):
+            rm_p(output_file)
+
+        # execute BUFR2IODAX to convert snocvr bufr data into IODA format
+        yaml_file = f"bufr_snocvr.yaml"
+        if not os.path.isfile(f"{os.path.join(localconf.DATA, yaml_file)}"):
+            logger.exception(f"{yaml_file} not found")
+            raise FileNotFoundError(f"{os.path.join(localconf.DATA, yaml_file)}")
+        exe = Executable(self.task_config.BUFR2IODAX)
+        exe.add_default_arg(os.path.join(localconf.DATA, f"{yaml_file}"))
+


I have a suggestion to remove some repetition here. Please see the patch on this file if you could kindly test.
diff.txt

I just tested this, and it worked perfect. The updates were committed to the repo. Thanks @aerorahul for your suggestions.

aerorahul · 2023-09-08T19:42:10Z

ush/python/pygfs/task/land_analysis.py

+        # generate bufr2ioda YAML files
+        adpsfc_yaml = os.path.join(self.runtime_config.DATA, "bufr_adpsfc_snow.yaml")
+        logger.info(f"Generate BUFR2IODA YAML file: {adpsfc_yaml}")
+        temp_yaml = parse_j2yaml(self.task_config.BUFRADPSFCYAML, self.task_config)
+        save_as_yaml(temp_yaml, adpsfc_yaml)
+        logger.info(f"Wrote bufr2ioda YAML to: {adpsfc_yaml}")
+        snocvr_yaml = os.path.join(self.runtime_config.DATA, "bufr_snocvr.yaml")
+        logger.info(f"Generate BUFR2IODA YAML file: {snocvr_yaml}")
+        temp_yaml = parse_j2yaml(self.task_config.BUFRSNOCVRYAML, self.task_config)
+        save_as_yaml(temp_yaml, snocvr_yaml)
+        logger.info(f"Wrote bufr2ioda YAML to: {snocvr_yaml}")


I think we may be able to make this a bit less hard-coded and yaml driven from the top.
Say for e.g. somewhere in GDASApp/parm/land has bufr2ioda.yaml that contains:

bufr2ioda: adpsfc: {{ HOMEgfs }}/sorc/gdas.cd/test/testinput/bufr_adpsfc_snow.yaml snocvr: {{ HOMEgfs }}/sorc/gdas.cd/test/testinput/bufr_snocvr.yaml

We could then loop over this list to parse the yamls, and call the bufr2ioda.x and verify the output is produced.

The code then does not know what it is processing and bufr files can be added/removed to the yaml.

Would this be acceptable?

we may be able to use land/prep/prep_gts.yaml as it already seems to have the appropriate information and it is likely better to keep them together.

@aerorahul Follow your suggestions, I have made the changes for your review. Thanks.

aerorahul · 2023-09-11T20:47:21Z

ush/python/pygfs/task/land_analysis.py

+        # generate bufr2ioda YAML files
+        for name in ["adpsfc", "snocvr"]:
+            gts_yaml = os.path.join(self.runtime_config.DATA, f"bufr_{name}_snow.yaml")
+            logger.info(f"Generate BUFR2IODA YAML file: {gts_yaml}")
+            temp_yaml = parse_j2yaml(prep_gts_config.bufr2ioda[name], localconf)
+            save_as_yaml(temp_yaml, gts_yaml)
+            logger.info(f"Wrote bufr2ioda YAML to: {gts_yaml}")


Thanks for updating this section @jiaruidong2017
Here is what I was suggesting.
In the attached patch, you will notice that I have removed any knowledge of what is being converted from bufr to ioda in the python script. The control comes directly from the yaml.

Would you be willing to give this a try?
diff.txt

Thanks

Thanks @aerorahul. I will try it and let you know then.

@aerorahul I just tested and it worked perfectly. The changes have been committed to the repo.

aerorahul · 2023-09-11T22:16:31Z

ush/python/pygfs/task/land_analysis.py

-        # execute BUFR2IODAX to convert snocvr bufr data into IODA format
-        _gtsbufr2iodax(exe, os.path.join(localconf.DATA, "bufr_snocvr_snow.yaml"))
+            # execute BUFR2IODAX to convert {name} bufr data into IODA format
+            _gtsbufr2iodax(exe, os.path.join(localconf.DATA, f"bufr_{name}_snow.yaml"))


Suggested change

_gtsbufr2iodax(exe, os.path.join(localconf.DATA, f"bufr_{name}_snow.yaml"))

_gtsbufr2iodax(exe, gts_yaml)

In my second (revised diff.txt), I removed the need to construct this again.

Done the changes.

aerorahul

Looks good.
Thank you for accepting suggestions.
If I could make one more suggestion that aids in knowing the inputs of this method completely, is to consolidate everything that is needed into localconf and then only use localconf everywhere.
At present, you will notice, variables are being accessed from task_config, runtime_config as well as localconf.

jiaruidong2017 · 2023-09-11T22:41:49Z

Looks good. Thank you for accepting suggestions. If I could make one more suggestion that aids in knowing the inputs of this method completely, is to consolidate everything that is needed into localconf and then only use localconf everywhere. At present, you will notice, variables are being accessed from task_config, runtime_config as well as localconf.

Thanks @aerorahul for the detailed instructions, and I have benefited a lot when I was working with you on this PR.

jiaruidong2017 · 2023-09-11T22:43:45Z

@aerorahul This PR depends on the NOAA-EMC/GDASApp#602. Would you please take time to review it? Thanks again.

aerorahul · 2023-09-13T13:16:06Z

@jiaruidong2017
Let us know when you want us to merge this PR. It is good to go from this side.

jiaruidong2017 · 2023-09-13T13:29:02Z

@aerorahul we need to wait for @CoryMartin-NOAA to merge the recent updates in GDASApp into the feature/stable-nightly, and update the commit hash in the global-workflow. I will let you know when it is ready. Thanks.

CoryMartin-NOAA · 2023-09-13T13:36:04Z

@jiaruidong2017 already done... d347d22

jiaruidong2017 · 2023-09-13T13:55:24Z

Thanks @CoryMartin-NOAA I have updated the commit hash for the GDASApp.
@aerorahul @WalterKolczynski-NOAA It is ready to merge this PR. Thanks.

requested changed were made.

jiaruidong2017 added 3 commits August 23, 2023 08:38

Initialize the branch to do the madis DA.

3a723e8

Merge branch 'develop' into feature/madis_da

46577a1

Merge branch 'develop' into feature/madis_da

32e24b4

jiaruidong2017 mentioned this pull request Sep 5, 2023

Add the capability to assimilate the MADIS snow depth for global-workflow NOAA-EMC/GDASApp#602

Merged

WalterKolczynski-NOAA previously requested changes Sep 8, 2023

View reviewed changes

WalterKolczynski-NOAA requested a review from aerorahul September 8, 2023 19:22

aerorahul reviewed Sep 8, 2023

View reviewed changes

jiaruidong2017 added 4 commits September 9, 2023 11:48

Remove some repetitions.

7f44dd4

Fix the python norm error.

0273248

Simplify the code to generate bufr2ioda YAML files

4d3e105

Remove the obsolete BUFR2IODAYAML and BUFRADPSFCYAML.

9bce693

jiaruidong2017 requested a review from aerorahul September 9, 2023 23:05

jiaruidong2017 added 2 commits September 10, 2023 20:12

Merge branch 'develop' into feature/madis_da

0017549

Address reviewer's comments.

60567df

jiaruidong2017 requested a review from WalterKolczynski-NOAA September 11, 2023 00:32

aerorahul reviewed Sep 11, 2023

View reviewed changes

Simplify the codes following the reviewer's suggestions.

dc83591

aerorahul reviewed Sep 11, 2023

View reviewed changes

Remove the unnecessary construction of gts_yaml.

94a0ff8

aerorahul approved these changes Sep 11, 2023

View reviewed changes

Merge branch 'develop' into feature/madis_da

04f7ec5

Update the GDASApp's commit hash.

73fcc1c

aerorahul merged commit ffa9445 into NOAA-EMC:develop Sep 13, 2023
4 checks passed

jiaruidong2017 deleted the feature/madis_da branch September 13, 2023 14:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the capability to assimilate the MADIS snow depth data from GTS #1836

Add the capability to assimilate the MADIS snow depth data from GTS #1836

jiaruidong2017 commented Sep 5, 2023 •

edited

Loading

WalterKolczynski-NOAA left a comment

WalterKolczynski-NOAA Sep 8, 2023

jiaruidong2017 Sep 11, 2023

aerorahul left a comment

aerorahul Sep 8, 2023

jiaruidong2017 Sep 9, 2023

aerorahul Sep 8, 2023

jiaruidong2017 Sep 9, 2023

aerorahul Sep 8, 2023

aerorahul Sep 8, 2023

jiaruidong2017 Sep 9, 2023

aerorahul Sep 11, 2023 •

edited

Loading

jiaruidong2017 Sep 11, 2023

jiaruidong2017 Sep 11, 2023

aerorahul Sep 11, 2023

jiaruidong2017 Sep 11, 2023

aerorahul left a comment

jiaruidong2017 commented Sep 11, 2023

jiaruidong2017 commented Sep 11, 2023

aerorahul commented Sep 13, 2023

jiaruidong2017 commented Sep 13, 2023

CoryMartin-NOAA commented Sep 13, 2023

jiaruidong2017 commented Sep 13, 2023

		if os.path.isfile(f"{os.path.join(localconf.DATA, output_file)}"):
		rm_p(output_file)

		export BUFRADPSFCYAML="${HOMEgfs}/sorc/gdas.cd/test/testinput/bufr_adpsfc_snow.yaml"
		export BUFRSNOCVRYAML="${HOMEgfs}/sorc/gdas.cd/test/testinput/bufr_snocvr.yaml"

	_gtsbufr2iodax(exe, os.path.join(localconf.DATA, f"bufr_{name}_snow.yaml"))
	_gtsbufr2iodax(exe, gts_yaml)

Add the capability to assimilate the MADIS snow depth data from GTS #1836

Add the capability to assimilate the MADIS snow depth data from GTS #1836

Conversation

jiaruidong2017 commented Sep 5, 2023 • edited Loading

How has this been tested?

Checklist

WalterKolczynski-NOAA left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aerorahul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aerorahul Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aerorahul left a comment

Choose a reason for hiding this comment

jiaruidong2017 commented Sep 11, 2023

jiaruidong2017 commented Sep 11, 2023

aerorahul commented Sep 13, 2023

jiaruidong2017 commented Sep 13, 2023

CoryMartin-NOAA commented Sep 13, 2023

jiaruidong2017 commented Sep 13, 2023

jiaruidong2017 commented Sep 5, 2023 •

edited

Loading

aerorahul Sep 11, 2023 •

edited

Loading