Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

resolve inconsistencies between bufr2ioda python converters and jcb templates #1241

Merged
merged 10 commits into from
Aug 12, 2024
Merged

resolve inconsistencies between bufr2ioda python converters and jcb templates #1241

merged 10 commits into from
Aug 12, 2024

Conversation

RussTreadon-NOAA
Copy link
Contributor

@RussTreadon-NOAA RussTreadon-NOAA commented Jul 31, 2024 •
edited
Loading

bufr2ioda python converters use different naming conventions for output ioda files. JCB templates assume certain patterns. This PR updates the output ioda filenames in bufr2ioda python converters to be consistent with JCB templates.

This PR focuses on bufr2ioda python scripts for select observation types used in atmospheric variational and local ensemble DA. Marine DA converters are not altered in this PR.

Resolves #1227

@RussTreadon-NOAA RussTreadon-NOAA self-assigned this Jul 31, 2024
@RussTreadon-NOAA RussTreadon-NOAA marked this pull request as draft July 31, 2024 22:45
@RussTreadon-NOAA
Copy link
Contributor Author

Convert to draft until in office to actively test.

@emilyhcliu
Copy link
Collaborator

Convert to draft until in office to actively test.

@RussTreadon-NOAA I can test this and report the test results here.
I will do this during JEDI Junction time tomorrow.

@RussTreadon-NOAA
Copy link
Contributor Author

@emilyhcliu , thank you. I have already tested most of the modified python bufr2ioda converters in this PR via global-workflow job prepatmiodaobs. The generated ioda files have been processed by the g-w JEDI ATM variational and local ensemble DA apps.

There is no science in this PR. It simply aligns atmospheric bufr2ioda output filenames with assumed patterns in JCB templates. No changes are made to marine bufr2ioda python converter scripts (ie, those in /ush/ioda/bufr2ioda/marine).

@RussTreadon-NOAA
Copy link
Contributor Author

RussTreadon-NOAA commented Aug 1, 2024 •
edited
Loading

Mark PR as Ready for review so @emilyhcliu can comment & review.

@RussTreadon-NOAA RussTreadon-NOAA marked this pull request as ready for review August 1, 2024 09:39
@RussTreadon-NOAA
Copy link
Contributor Author

RussTreadon-NOAA commented Aug 5, 2024 •
edited
Loading

@YoulongXia-NOAA and @jiaruidong2017 , ush/ioda/bufr2ioda/bufr2ioda_snocvr_bufr.py defines iodafile as

    iodafile = f"{cycle_type}.t{hh}z.{data_type}.nc"

I see that ./snow/jcb-base.yaml.j2 contains

atm_obsdatain_suffix: ".tm00.nc"

Given this, it seems we need to replace data_type in the iodafile line above with

    iodafile = f"{cycle_type}.t{hh}z.{data_type}.tm00.nc"

Do you agree?

@YoulongXia-NOAA
Copy link
Collaborator

@RussTreadon-NOAA, I am fine for a test as I just develop ioda-converters for snowcvr data. Let Jiarui @jiaruidong2017 to reply your question more precisely as he is working on a combined snow dataset and his work is more associated with GDASApp and global workflow tasks. Thank you.

@jiaruidong2017
Copy link
Collaborator

@RussTreadon-NOAA I am fine with any changes to the filename format as long as it keeps consistent.

When I check the iodafile in the ush/ioda/bufr2ioda/bufr2ioda_snocvr_bufr.py, the current form is iodafile = f"{cycle_type}.t{hh}z.{data_type}.{data_format}.nc" with .{data_format} there.

@jiaruidong2017
Copy link
Collaborator

@RussTreadon-NOAA The bufrfile in ush/ioda/bufr2ioda/bufr2ioda_snocvr_bufr.py is defined as below:

bufrfile = f"{cycle_type}.t{hh}z.{data_type}.tm00.{data_format}"
including tm00.

@jiaruidong2017
Copy link
Collaborator

@RussTreadon-NOAA I am fine with any changes to the filename format as long as it keeps consistent.

When I check the iodafile in the ush/ioda/bufr2ioda/bufr2ioda_snocvr_bufr.py, the current form is iodafile = f"{cycle_type}.t{hh}z.{data_type}.{data_format}.nc" with .{data_format} there.

@RussTreadon-NOAA The iodafile need to change from:

iodafile = f"{cycle_type}.t{hh}z.{data_type}.{data_format}.nc"

TO:

iodafile = f"{cycle_type}.t{hh}z.{data_type}.{data_format}"

@RussTreadon-NOAA
Copy link
Contributor Author

This is interesting, @jiaruidong2017 .

File parm/snow/jcb-prototype_3dvar.yaml.j2 contains

# Naming conventions for observational files
atm_obsdatain_path: "{{atm_obsdatain_path}}"
atm_obsdatain_prefix: "{{OPREFIX}}"
atm_obsdatain_suffix: ".tm00.nc"

This suggests that the suffix should be .tm00.nc.

Given your input I will not modify ush/ioda/bufr2ioda/bufr2ioda_snocvr_bufr.py in this PR. If needed, you can update this converter in a separate PR.

@jiaruidong2017
Copy link
Collaborator

jiaruidong2017 commented Aug 5, 2024 •
edited
Loading

Thanks @RussTreadon-NOAA We didn't use bufr2ioda_snocvr_bufr.py yet to convert SNOCVR BUFR data into ioda file for the CI test. You can make the changes in your PR, and we can follow the changes to define the ioda file later.

@RussTreadon-NOAA
Copy link
Contributor Author

@emilyhcliu , for the 2024050218 gdas, the prepatmiodaobs job creates the following ioda format dump files

gdas.t18z.ADPSFC.tm00.nc
gdas.t18z.ADPUPA.tm00.nc
gdas.t18z.SFCSHP.tm00.nc
gdas.t18z.acft_profiles.tm00.nc
gdas.t18z.ascatw.ascat_metop-b.tm00.nc
gdas.t18z.ascatw.ascat_metop-c.tm00.nc
gdas.t18z.atms_n20.tm00.nc
gdas.t18z.atms_npp.tm00.nc
gdas.t18z.conventional_ps.tm00.nc
gdas.t18z.gnssro.tm00.nc
gdas.t18z.gpsro.tm00.nc
gdas.t18z.mtiasi_metop-b.tm00.nc
gdas.t18z.mtiasi_metop-c.tm00.nc
gdas.t18z.ompsnp_n20.tm00.nc
gdas.t18z.ompsnp_npp.tm00.nc
gdas.t18z.ompstc_n20.tm00.nc
gdas.t18z.ompstc_npp.tm00.nc
gdas.t18z.satwnd.abi_goes-16.tm00.nc
gdas.t18z.satwnd.abi_goes-18.tm00.nc
gdas.t18z.satwnd.leogeo_multi.tm00.nc
gdas.t18z.satwnd.viirs_n20.tm00.nc
gdas.t18z.satwnd.viirs_npp.tm00.nc

A few questions:

  1. We don't have yamls for all these dump files in parm/jcb-gdas/observations/atmosphere. Should we have an observation yaml for each ioda format dump file?
  2. Do we have a problem with inconsistency between the names of the ioda format dump files and the names of the observation yamls? For example, we have ioda format dump file SFCSHP and observation yaml sfcship. Do these two go together?
  3. We have yamls in parm/jcb-gdas/observations/atmosphere for which we do not have bufr2ioda converters in ush/ioda/bufr2ioda. Examples include cris-fsr-n20.yaml.j2 and ssmis_f17.yaml.j2. Is there no buf2ioda converter script for these observation types because the plan to directly read the bufr dump file via the ioda backend?

@RussTreadon-NOAA
Copy link
Contributor Author

Thanks @RussTreadon-NOAA We didn't use bufr2ioda_snocvr_bufr.py yet to convert SNOCVR BUFR data into ioda file for the CI test. You can make the changes in your PR, and we can follow the changes to define the ioda file later.

I am reluctant to change things I can not test. I will not modify bufr2ioda_snocvr_bufr.py in this PR.

@emilyhcliu
Copy link
Collaborator

emilyhcliu commented Aug 5, 2024 •
edited
Loading

@RussTreadon-NOAA. I break down the list of observations from you into two lists (A and B)

Here is the list of observations - List-A
These observations were validated in the end-to-end test before the implementation of JCB.

gdas.t18z.ascatw.ascat_metop-b.tm00.nc
gdas.t18z.ascatw.ascat_metop-c.tm00.nc
gdas.t18z.mtiasi_metop-b.tm00.nc
gdas.t18z.mtiasi_metop-c.tm00.nc
gdas.t18z.atms_n20.tm00.nc
gdas.t18z.atms_npp.tm00.nc
gdas.t18z.conventional_ps.tm00.nc
gdas.t18z.gnssro.tm00.nc
gdas.t18z.gpsro.tm00.nc
gdas.t18z.ompsnp_n20.tm00.nc
gdas.t18z.ompsnp_npp.tm00.nc
gdas.t18z.ompstc_n20.tm00.nc
gdas.t18z.ompstc_npp.tm00.nc
gdas.t18z.satwnd.abi_goes-16.tm00.nc
gdas.t18z.satwnd.abi_goes-18.tm00.nc
gdas.t18z.satwnd.leogeo_multi.tm00.nc
gdas.t18z.satwnd.viirs_n20.tm00.nc
gdas.t18z.satwnd.viirs_npp.tm00.nc

Here is the list of observations - List-B
These are conventional data. Andrew is leading the evaluation of these observation types.

gdas.t18z.ADPSFC.tm00.nc
gdas.t18z.ADPUPA.tm00.nc
gdas.t18z.SFCSHP.tm00.nc
gdas.t18z.acft_profiles.tm00.nc

So, Let's deal with the observations in List-A first.
For observation in List-A, the conventional_ps is tested with JCB and will be modified to your way in this PR.
For the other observation in List-A, I opened Issue #1240.
I will check one by one to make sure they work properly with JCB and update filter options such as reduce obs space.
With the reduced obs space, cycling DA is more feasible (e.g., for IASI, only data after thinning will be kept in the obs space for variational analysis).

Here are slides regarding reduce obs space I shared last Thursday during EMC JEDI Junction

20240731 JEDI T2O
20240731 JEDI T2O (3)

Am I heading in the right direction?

@RussTreadon-NOAA
Copy link
Contributor Author

Thank you @emilyhcliu for the update and the work you are doing in issue #1240.

I'll continue this PR by focusing on what I can get through prepatmiodaobs, atmanlinit, and atmanlvar via minor script and filename changes. I will probably hold off on local ensemble DA until g-w issue #2415 has a PR.

@emilyhcliu emilyhcliu mentioned this pull request Aug 6, 2024
Open
9 tasks
@RussTreadon-NOAA RussTreadon-NOAA added the hera-GW-RT Queue for automated testing with global-workflow on Hera label Aug 8, 2024
@emcbot emcbot added hera-GW-RT-Running Automated testing with global-workflow running on Hera and removed hera-GW-RT Queue for automated testing with global-workflow on Hera labels Aug 8, 2024
@emcbot emcbot removed the hera-GW-RT-Running Automated testing with global-workflow running on Hera label Aug 8, 2024
@RussTreadon-NOAA
Copy link
Contributor Author

Automated testing on Hera and manual testing on Hercules yield Passed for ctests. The changes in this PR have been cycled in parallels on Hera and Hercules. This PR is ready for review.

@RussTreadon-NOAA
Copy link
Contributor Author

@emilyhcliu , are there any UFO validation developers you think should review this PR?

emilyhcliu
emilyhcliu previously approved these changes Aug 8, 2024
View reviewed changes
@RussTreadon-NOAA
Copy link
Contributor Author

Thank you @emilyhcliu

XuanliLi-NOAA
XuanliLi-NOAA previously approved these changes Aug 9, 2024
View reviewed changes
Copy link
Collaborator

@XuanliLi-NOAA XuanliLi-NOAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me.

@RussTreadon-NOAA
Copy link
Contributor Author

Thank you @XuanliLi-NOAA

emilyhcliu
emilyhcliu previously approved these changes Aug 9, 2024
View reviewed changes
@XuanliLi-NOAA
Copy link
Collaborator

@RussTreadon-NOAA, with your new changes, the json file needs to be renamed to bufr2ioda_gnssro.json (currently bufr2ioda_gnssro_bufr.json) and the bufr2ioda python script needs to be renamed to bufr2ioda_gnssro.py (currently bufr2ioda_gnssro_bufr.py).

@RussTreadon-NOAA
Copy link
Contributor Author

@XuanliLi-NOAA , I made the requested change in a working copy of feature/bufr2ioda on Hercules. prepatmiodaobs works fine with the name change.

I have a question.

parm/ioda/bufr2ioda and ush/ioda/bufr2ioda contains the following pair of files

  • parm: bufr2ioda_gnssro.json and bufr2ioda_gpsro_bufr_combined.json
  • ush: bufr2ioda_gnssro.py and bufr2ioda_gpsro_bufr_combined.py

Both python scripts read the same input bufrfile, namely ${cdump}.t${cyc}z.gpsro.tm00.bufr_d, and write to different output iodafile

  • bufr2ioda_gnssro.py creates ${cdump}.t${cyc}z.gnssro.tm00.nc
  • bufr2ioda_gpsro_bufr_combined.py creates ${cdump}.t${cyc}z..gpsro.tm00.nc

I see both gnssro.tm00.nc and gpsro.tm00.nc created when I run prepatmiodaobs.

However, there is no observation yaml for gpsro. The jcb-gdas repo only contains gnssro.yaml.j2 in observations/atmosphere and observations/atmosphere-lgetk. There are no jcb-gdas yamls for gpsro in either observations/atmosphere or observations/atmosphere-lgetkf.

Do we need the gpsro bufr2ioda converter and json file?

  • If yes, it seems we should add gpsro yamls to jcb-gdas.
  • If no, it seems we should remove the gpsro bufr2ioda converter and json file.

What do you think?

@XuanliLi-NOAA
Copy link
Collaborator

bufr2ioda_gpsro_bufr_combined.json and bufr2ioda_gpsro_bufr_combined.py are old files. We no longer need them.

@RussTreadon-NOAA
Copy link
Contributor Author

Thank @XuanliLi-NOAA for letting me know that the two files in question are no longer needed. Both files have been removed. Done at 8d842b2.

@XuanliLi-NOAA
Copy link
Collaborator

Thank you!

@RussTreadon-NOAA
Copy link
Contributor Author

@emilyhcliu , @XuanliLi-NOAA , and @PraveenKumar-NOAA : do you have any more comments or requests for this PR? If not, would you please review and approve. We would like to merge this PR into develop.

PraveenKumar-NOAA
PraveenKumar-NOAA approved these changes Aug 10, 2024
View reviewed changes
Copy link
Collaborator

@PraveenKumar-NOAA PraveenKumar-NOAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

parm/jcb-gdas Outdated Show resolved Hide resolved
XuanliLi-NOAA
XuanliLi-NOAA approved these changes Aug 11, 2024
View reviewed changes
Copy link
Collaborator

@XuanliLi-NOAA XuanliLi-NOAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@RussTreadon-NOAA
Copy link
Contributor Author

RussTreadon-NOAA commented Aug 11, 2024 •
edited
Loading

Thank you @XuanliLi-NOAA

@CoryMartin-NOAA CoryMartin-NOAA merged commit 9e38433 into NOAA-EMC:develop Aug 12, 2024
2 checks passed
@RussTreadon-NOAA
Copy link
Contributor Author

Thank you @CoryMartin-NOAA . I was just looking at this PR and the status changed. Wow!

@RussTreadon-NOAA RussTreadon-NOAA deleted the feature/bufr2ioda branch August 12, 2024 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PraveenKumar-NOAA PraveenKumar-NOAA PraveenKumar-NOAA approved these changes

@XuanliLi-NOAA XuanliLi-NOAA XuanliLi-NOAA approved these changes

@emilyhcliu emilyhcliu emilyhcliu left review comments

Assignees

@RussTreadon-NOAA RussTreadon-NOAA

Labels
hera-GW-RT-Passed Automated testing with global-workflow successful on Hera
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Inconsistency between bufr2ioda iodafile and init obsdatain
8 participants
@RussTreadon-NOAA @emilyhcliu @YoulongXia-NOAA @jiaruidong2017 @emcbot @XuanliLi-NOAA @PraveenKumar-NOAA @CoryMartin-NOAA