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

Convert the ECMWF model level products to pressure level products #31

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

xhuang-jpl
Copy link

@xhuang-jpl xhuang-jpl commented Jan 30, 2024

@scottstanie @mirzaees @vbrancat

To make the pyAPS accept the ECMWF model level products, this PR is to convert the model level ECMWF products in NetCDF format to the pressure level products. Both the theory and partial codes for the conversion can be found in ECMWF website. This PR is tested comparing with the ECMWF pressure level products. Some tests results are shown as,

1. Temperature

image

2. Specific Humidity

image

3. Geopotential

image

@vbrancat
Copy link

@scottstanie @mirzaaes can you please review this PR? We would like to have a quick turn around on this and I believe this PR should be also beneficial for your projects. Thank you.

Copy link

@mirzaees mirzaees left a comment

Choose a reason for hiding this comment

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

Thank you @xhuang-jpl, this is a useful PR specially once we start using HRES data. It looks good to me, however it would be nice if you could add docstring and parameter description for those functions that they don't have

count = 0
profile_count = 0
interpolated_values=[]
for point in range(no_grid_points):

Choose a reason for hiding this comment

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

where is variable point being used?

Copy link
Author

Choose a reason for hiding this comment

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

This variable is not used, and will simply replace it with _

Comment on lines +49 to +60
def get_input_variable_list(fin):
ds = xr.open_dataset(fin)
var_list = list(ds.keys())
ds = None
var_list_unique = list(set(var_list))
if 'lnsp' not in var_list_unique:
print("Error - lnsp variable missing from input file -exiting")
sys.exit()
if len(var_list_unique) < 2:
print("Error - Data variable missing from input file -exiting")
sys.exit()
return var_list_unique
Copy link
Contributor

Choose a reason for hiding this comment

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

A couple general comments which carry throughout the module-

  • For these general functions, let's raise a meaningful exception rather than call sys.exit
  • you appear to sometimes use 2 spaces for indentation rather than 4
  • a docstring describing the inputs/outputs would be helpful. for this function, it's hard to guess just from looking at these 10 lines what fin should be, or where i could find a file with 'lnsp' as an xarray-readable variable
  • As we mentioned offline, let's double check- is xarray needed for this? or is it just used to read in the inputs (which are GRIB files readable by pygrib?) and write the outputs

For this specific function

  • this function just open a file and gets the variables, but then we reopen and use the file right after
  • it we're doing this anyway, we should use the context manager version of with xr.open_dataset(fin):... . It's only the Python-gdal bindings that require you to do ds = None to close a dataset
  • I can't tell what var_list_unique is supposed to contain that is a problem if the length is < 2

Copy link
Author

Choose a reason for hiding this comment

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

Thank you @scottstanie, I think we might need the netcdf reader here because the NISAR PCM has already the interface to generate the netcdf to feed the ISCE3, let me find a cheaper way to do this, maybe only the netcdf reader instead of the xarray.

@xhuang-jpl
Copy link
Author

I am a little concerned about how to deal with those NaNs that are out range of the pressure, interestingly, the downloaded pressure level products do not have this issue.

I am thinking two options,
Option 1: Interpolating them, in this way, we might introduce more delays.
Option 2: Skip the NaNs when calculate the delay, in which we might need more code change of the pyAPS to deal with NaNs.

@scottstanie
Copy link
Contributor

Since there is already GRIB test data in the repo, it should be quick to add a test of the main conversion function

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.

4 participants