Prepare GTS snow depth observations for JEDI-based Land DA #1761

jiaruidong2017 · 2023-07-23T19:51:31Z

This PR:

adds a job to prepare GTS snow depth observations as a task in the workflow. This task depends on the prep.sh job to bring GTS adpsfc bufr data. To test this type of data DMPDIR in config.base needed to be pointed to "/scratch1/NCEPDEV/global/Jiarui.Dong/JEDI/GlobalWorkflow/para_gfs/glopara_dump".
land_analysis.py introduces a method prepare_GTS for this type of data.
Updates are necessary in the GDASApp repo. See companion PR Prepare GTS snow depth observations for JEDI-based Land DA GDASApp#541

The preplandobs job runs at the all four cycles in the workflow. The prepare_IMS job runs only at 18z cycle, and this is controlled in the script.

Checklist

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 need updates to the documentation. I have made corresponding changes to the documentation
My changes generate no new warnings
New and existing tests pass with my changes
Any dependent changes have been merged and published

CoryMartin-NOAA

Couple of minor comments

CoryMartin-NOAA · 2023-07-24T15:00:39Z

scripts/exglobal_prep_land_obs.py

@@ -17,7 +17,10 @@

    # Take configuration from environment and cast it as python dictionary
    config = cast_strdict_as_dtypedict(os.environ)
+    HH = os.environ.get("CDATE")[8:]


isn't there cyc?

I just checked the cyc is not defined as below:

print(f"cyc: {cyc}") NameError: name 'cyc' is not defined

@aerorahul is going to have a preferred solution for this.

CoryMartin-NOAA · 2023-07-24T15:04:12Z

ush/python/pygfs/task/land_analysis.py

+        logger.debug(f"{self.task_config.GTS_OBS_LIST}:\n{pformat(prep_gts_config)}")
+
+        # copy the GTS obs files from COM_OBS to DATA/obs
+        logger.info("Copying GTS obs for BUFR2IODAX")


suggest BUFR2IODAX to bufr2ioda.x

Made the change.

aerorahul

Mostly looks great. Just some minor points.
Now that there are multiple observation processing steps, we may want to bring those methods out in the near future to keep DA and preprocessing separate to enable testing.

aerorahul · 2023-07-24T20:16:44Z

scripts/exglobal_prep_land_obs.py

@@ -17,7 +17,10 @@

    # Take configuration from environment and cast it as python dictionary
    config = cast_strdict_as_dtypedict(os.environ)
+    HH = os.environ.get("CDATE")[8:]


Suggested change

HH = os.environ.get("CDATE")[8:]

There is no need to go to os.environ after creating config from the environment.

Done as suggested.

aerorahul · 2023-07-24T20:19:24Z

scripts/exglobal_prep_land_obs.py


    # Instantiate the land prepare task
    LandAnl = LandAnalysis(config)
-    LandAnl.prepare_IMS()
+    LandAnl.prepare_GTS()
+    if HH == '18':


@CoryMartin-NOAA I think you are thinking of this.

Suggested change

if HH == '18':

if LandAnl.runtime_config.cyc == '18':

Made the changes. Thanks @aerorahul

aerorahul · 2023-07-24T20:21:35Z

ush/python/pygfs/task/land_analysis.py

@@ -35,6 +35,7 @@ def __init__(self, config):
        _res = int(self.config['CASE'][1:])
        _window_begin = add_to_datetime(self.runtime_config.current_cycle, -to_timedelta(f"{self.config['assim_freq']}H") / 2)
        _letkfoi_yaml = os.path.join(self.runtime_config.DATA, f"{self.runtime_config.RUN}.t{self.runtime_config['cyc']:02d}z.letkfoi.yaml")
+        _bufr2ioda_yaml = os.path.join(self.runtime_config.DATA, f"bufr_adpsfc_snow.yaml")


Suggested change

_bufr2ioda_yaml = os.path.join(self.runtime_config.DATA, f"bufr_adpsfc_snow.yaml")

I need to think of this a bit. I don't think this belongs in the constructor.

Removed as suggested.

aerorahul · 2023-07-24T20:21:48Z

ush/python/pygfs/task/land_analysis.py

@@ -47,13 +48,89 @@ def __init__(self, config):
                'LAND_WINDOW_LENGTH': f"PT{self.config['assim_freq']}H",
                'OPREFIX': f"{self.runtime_config.RUN}.t{self.runtime_config.cyc:02d}z.",
                'APREFIX': f"{self.runtime_config.RUN}.t{self.runtime_config.cyc:02d}z.",
-                'jedi_yaml': _letkfoi_yaml
+                'jedi_yaml': _letkfoi_yaml,
+                'bufr2ioda_yaml': _bufr2ioda_yaml


Suggested change

'bufr2ioda_yaml': _bufr2ioda_yaml

Removed as suggested.

aerorahul · 2023-07-24T20:23:03Z

ush/python/pygfs/task/land_analysis.py

+        # generate bufr2ioda YAML file
+        logger.info(f"Generate BUFR2IODA YAML file: {self.task_config.bufr2ioda_yaml}")
+        bufr2ioda_yaml = parse_j2yaml(self.task_config.BUFR2IODAYAML, self.task_config)
+        save_as_yaml(bufr2ioda_yaml, self.task_config.bufr2ioda_yaml)


You can just define the name of the file here and save it.

As suggested, the bufr2ioda_yaml is defined locally.

jiaruidong2017 · 2023-07-24T22:48:24Z

Mostly looks great. Just some minor points. Now that there are multiple observation processing steps, we may want to bring those methods out in the near future to keep DA and preprocessing separate to enable testing.

Thanks @aerorahul for your review. I made the changes as suggested, and I have tested the changes successfully.

jiaruidong2017 · 2023-07-24T22:51:03Z

@CoryMartin-NOAA Would you please merge develop to feature/stable-nightly in GDASApp so that I can update the GDASApp's version here. Thanks.

aerorahul

Thank you for updating so quickly.
Look great.

CoryMartin-NOAA · 2023-07-25T12:19:03Z

@jiaruidong2017 NOAA-EMC/GDASApp@7966501 was merged in manually

jiaruidong2017 · 2023-07-25T12:30:53Z

Thanks @CoryMartin-NOAA, and I have updated the GDASApp's commit hash.

WalterKolczynski-NOAA · 2023-07-25T17:59:45Z

ush/python/pygfs/task/land_analysis.py

+        if not os.path.isfile(f"{os.path.join(localconf.DATA, output_file)}"):
+            logger.exception(f"{self.task_config.BUFR2IODAX} failed to produce {output_file}")
+            raise FileNotFoundError(f"{os.path.join(localconf.DATA, output_file)}")
+        else:
+            logger.info(f"Copy {output_file} to {self.task_config.COM_OBS}")
+            FileHandler(prep_gts_config.gtsioda).sync()


There's no need to do the extra check here. If the source file doesn't exist, FileHandler.sync() will already raise an exception. We can just call the method and let the Exception rise through if it occurs. sync() also already writes the copy info to the logger, so we don't need to repeat that here.

Suggested change

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

logger.exception(f"{self.task_config.BUFR2IODAX} failed to produce {output_file}")

raise FileNotFoundError(f"{os.path.join(localconf.DATA, output_file)}")

else:

logger.info(f"Copy {output_file} to {self.task_config.COM_OBS}")

FileHandler(prep_gts_config.gtsioda).sync()

FileHandler(prep_gts_config.gtsioda).sync()

If you really want to add the message about BUFR2IODAX, just catch the exception briefly:

Suggested change

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

logger.exception(f"{self.task_config.BUFR2IODAX} failed to produce {output_file}")

raise FileNotFoundError(f"{os.path.join(localconf.DATA, output_file)}")

else:

logger.info(f"Copy {output_file} to {self.task_config.COM_OBS}")

FileHandler(prep_gts_config.gtsioda).sync()

try:

FileHandler(prep_gts_config.gtsioda).sync()

except OSError err:

logger.exception(f"{self.task_config.BUFR2IODAX} failed to produce {output_file}")

raise Exception(err)

Thanks @WalterKolczynski-NOAA for your review and suggestions. I made and committed the changes.

aerorahul · 2023-07-25T19:19:47Z

ush/python/pygfs/task/land_analysis.py

+            FileHandler(prep_gts_config.gtsioda).sync()
+        except OSError:
+            logger.exception(f"{self.task_config.BUFR2IODAX} failed to produce {output_file}")
+            raise Exception(err)


Suggested change

raise Exception(err)

raise OSError(err)

Raise the appropriate exception not the generic exception.
Also, err is undefined.
This will not work.

Sorry about that.

aerorahul · 2023-07-25T19:20:45Z

ush/python/pygfs/task/land_analysis.py

+        # If so, copy to COM_OBS/
+        try:
+            FileHandler(prep_gts_config.gtsioda).sync()
+        except OSError:


Suggested change

except OSError:

except OSError as err:

Thanks @aerorahul and made a fix as suggested.

WalterKolczynski-NOAA

Conditionally approved pending CI success.

emcbot · 2023-07-25T20:43:42Z