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.

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

Add logic for running offline IODA tools. #600

Merged

Conversation

delippi
Copy link
Contributor

@delippi delippi commented Jan 24, 2025

DESCRIPTION OF CHANGES:

  • Added logic to exrrfs_ioda_bufr.sh to run an offline tool to process IODA files. The offline tool adds MetaData/longitude_latitude_pressure to the IODA file which is used as the group category for duplicate checking based on RDASApp#259
  • Also cleans up some a few things in the edited file including extra white space.

TESTS CONDUCTED:

Tested on Hera conus 12km retro runs.

ISSUE:

Dependencies:

Copy link
Contributor

@MatthewPyle-NOAA MatthewPyle-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, but will wait to hear from others before merging...

${cpreq} ${HOMErrfs}/sorc/RDASApp/rrfs-test/IODA/offline_add_var_to_ioda.py .
ioda_files=$(ls ioda*nc)
for ioda_file in ${ioda_files[@]}; do
python offline_add_var_to_ioda.py -o ${ioda_file}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think NCO wants the script (including *.py) to run from the command line directly without the leading "python".

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, has the Python environment been correctly loaded for this tool?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@guoqing-noaa, thanks for the comments. The tool already had #!/usr/bin/env python at the top. Its just habit to run it with python anyway. I just removed it in my most recent commit and it still works. As for the python environment it seems everything that is needed is already loaded so I didn't have to load anything else.

@MatthewPyle-NOAA
Copy link
Contributor

@delippi Did you test within the leading python to make sure it all still works? If so, think we can merge it.

@delippi
Copy link
Contributor Author

delippi commented Jan 24, 2025

@MatthewPyle-NOAA hold off on merging though. There are some dependencies.

Edit: The dependencies are documented in the initial comment. Please don't merge until those are satisfied, otherwise the python tool won't be available. I need some people to review that NOAA-EMC/RDASApp#259.

@delippi
Copy link
Contributor Author

delippi commented Jan 24, 2025

I also just noticed that this exrrfs_ioda_bufr.sh is not using the most up to date version of the bufr2ioda yamls. I actually have those saved in RDASApp. I think we ought to have those in a single location, but which repo? @hu5970 @MatthewPyle-NOAA @ShunLiu-NOAA

${cpreq} ${HOMErrfs}/sorc/RDASApp/rrfs-test/IODA/offline_add_var_to_ioda.py .
ioda_files=$(ls ioda*nc)
for ioda_file in ${ioda_files[@]}; do
offline_add_var_to_ioda.py -o ${ioda_file}
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for another comment, could you add ./ before offline_add_var_to_ioda.py?
Not all environments put the current directory in the $PATH variable.
(for example, our Jet role account cron jobs set $PATH to empty)

@guoqing-noaa
Copy link
Contributor

I also just noticed that this exrrfs_ioda_bufr.sh is not using the most up to date version of the bufr2ioda yamls. I actually have those saved in RDASApp. I think we ought to have those in a single location, but which repo? @hu5970 @MatthewPyle-NOAA @ShunLiu-NOAA

@delippi Thanks for noticing this! Yes, we will need to update the bufr2ioda yaml files.

For all PARM files, rrfs-workflow will be self-contained. That means, RDASApp manages its parm/test files while rrfs-workflow adapts a given version from RDASApp. This adheres to the NCO standard generating PARM files on the fly from the PARMrrfs directory and has the benefit that RDASApp can move forward with any new experimental changes without worrying about affecting rrfs-workflow (similar to our previous practice that the gsiparm.* files are in both the GSI repo and the rrfsv1 repo).

@guoqing-noaa
Copy link
Contributor

I would suggest we hold this PR until the discussion in NOAA-EMC/RDASApp#259 is resolved.

@delippi
Copy link
Contributor Author

delippi commented Feb 6, 2025

RDASApp was updated yesterday with NOAA-EMC/RDASApp#259, but in order for this PR to work, the version of RDASApp needs updated to the latest. Should I add that to this PR or should that be done separately?

@MatthewPyle-NOAA
Copy link
Contributor

@delippi I would say yes to updating the hash of RDASApp to pick up what is needed within this PR. Unless others object.

@guoqing-noaa
Copy link
Contributor

Yes, the same here. Vote for updating the RDASApp hash.

@delippi I would say yes to updating the hash of RDASApp to pick up what is needed within this PR. Unless others object.

@guoqing-noaa
Copy link
Contributor

This PR can be merged after #618 is merged,

@MatthewPyle-NOAA
Copy link
Contributor

All good on your end, @delippi ?

@delippi
Copy link
Contributor Author

delippi commented Feb 6, 2025

Wow, thanks for updating the hash to RDASApp @guoqing-noaa. Yeah, I think we are good to go then.

@MatthewPyle-NOAA MatthewPyle-NOAA merged commit a973ad0 into NOAA-EMC:rrfs-mpas-jedi Feb 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants